feat(authguest): atomic verify pin, rollback mechanism for renew

This commit is contained in:
Alex Ababii committed 2026-10-13 09:30:52 +02:00
1 parent 7a2639128d
commit 46dce34dd6
9 files changed
+252 -145

No files matched your search

@@ -37,6 +37,12 @@ func RedeemHandler(log log.Logger, s authguest.AuthGuest, cfg *config.Config) fu
return
}
if req.Token == "" {
log.Debug().Msg("request body is missing the token")
writeError(w, http.StatusBadRequest, errorResponse{ErrorType: "invalidRequest", Message: "The request body is missing the token."})
return
}
result, err := s.Redeem(r.Context(), req.Token)
if err != nil {
log.Debug().Err(err).Msg("redeem failed")
@@ -153,3 +153,19 @@ func TestRedeemHandlerMalformedBody(t *testing.T) {
require.NoError(t, json.NewDecoder(rr.Body).Decode(&resp))
assert.Equal(t, "invalidRequest", resp.ErrorType)
}
func TestRedeemHandlerMissingToken(t *testing.T) {
svcMock := mocks.NewAuthGuest(t)
body, err := json.Marshal(RedeemRequest{})
require.NoError(t, err)
rr := httptest.NewRecorder()
newRedeemHandler(t, svcMock)(rr, httptest.NewRequest(http.MethodPost, "/", strings.NewReader(string(body))))
assert.Equal(t, http.StatusBadRequest, rr.Code)
var resp errorResponse
require.NoError(t, json.NewDecoder(rr.Body).Decode(&resp))
assert.Equal(t, "invalidRequest", resp.ErrorType)
}
@@ -136,10 +136,6 @@ func (s *AuthGuestService) Redeem(ctx context.Context, tokenString string) (*Ses
// Renew generates a new guest link token and PIN for the given share.
func (s *AuthGuestService) Renew(ctx context.Context, shareID string) error {
if _, err := s.store.Get(token.Hash(shareID)); err != nil {
return &GuestError{ErrorType: err, ShareID: shareID}
}
if s.publisher == nil {
return ErrEventsNotConfigured
}
@@ -166,19 +162,20 @@ func (s *AuthGuestService) Renew(ctx context.Context, shareID string) error {
now := time.Now()
if err := s.store.Replace(storage.Record{
ShareID: shareID,
ShareIDHash: tok.ShareIDHash,
SecretHash: tok.SecretHash(),
PinHash: pinHash,
Expiry: now.Add(guestLinkTokenTTL),
PinExpiry: now.Add(guestPinTTL),
Redeemed: false,
var previous storage.Record
if err := s.store.Update(token.Hash(shareID), func(rec *storage.Record) error {
previous = *rec
rec.SecretHash = tok.SecretHash()
rec.PinHash = pinHash
rec.Expiry = now.Add(guestLinkTokenTTL)
rec.PinExpiry = now.Add(guestPinTTL)
rec.Redeemed = false
return nil
}); err != nil {
return err
return &GuestError{ErrorType: err, ShareID: shareID}
}
return revaevents.Publish(ctx, s.publisher, events.GuestTokenRenewed{
if err := revaevents.Publish(ctx, s.publisher, events.GuestTokenRenewed{
ShareID: share.GetId(),
Sharer: share.GetCreator(),
GranteeEmail: share.GetGrantee().GetUserId().GetOpaqueId(),
@@ -186,42 +183,51 @@ func (s *AuthGuestService) Renew(ctx context.Context, shareID string) error {
Token: tok.String(),
Pin: pinStr,
Timestamp: now,
})
}); err != nil {
if rbErr := s.store.Update(token.Hash(shareID), func(rec *storage.Record) error {
*rec = previous
return nil
}); rbErr != nil {
return fmt.Errorf("publishing guest token renewed event failed: %w (rollback failed: %v)", err, rbErr)
}
return err
}
return nil
}
// VerifyPin validates a PIN and its share and exchanges them for a session token
// and the share id.
func (s *AuthGuestService) VerifyPin(ctx context.Context, shareID, pinValue string) (*SessionResponse, error) {
rec, err := s.store.Get(token.Hash(shareID))
if err != nil {
if errors.Is(err, storage.ErrNotFound) {
// TODO: should a missing record map to 404 tokenNotFound instead of 401 pinInvalid?
return nil, &GuestError{ErrorType: ErrPinInvalid, ShareID: shareID}
}
return nil, &GuestError{ErrorType: err, ShareID: shareID}
}
if rec.PinHash == "" {
return nil, &GuestError{ErrorType: ErrPinInvalid, ShareID: shareID}
}
if !rec.PinExpiry.IsZero() && rec.PinExpiry.Before(time.Now()) {
return nil, &GuestError{ErrorType: ErrPinExpired, ShareID: shareID}
}
ok, err := pin.Verify(pinValue, rec.PinHash)
if err != nil || !ok {
return nil, &GuestError{ErrorType: ErrPinInvalid, ShareID: shareID}
}
if _, err := s.validateShare(ctx, shareID); err != nil {
return nil, err
}
rec.PinHash = ""
rec.PinExpiry = time.Time{}
if err := s.store.Replace(rec); err != nil {
return nil, err
err := s.store.Update(token.Hash(shareID), func(rec *storage.Record) error {
if rec.PinHash == "" {
return ErrPinInvalid
}
if !rec.PinExpiry.IsZero() && rec.PinExpiry.Before(time.Now()) {
return ErrPinExpired
}
ok, err := pin.Verify(pinValue, rec.PinHash)
if err != nil || !ok {
return ErrPinInvalid
}
rec.PinHash = ""
rec.PinExpiry = time.Time{}
return nil
})
if err != nil {
if errors.Is(err, storage.ErrNotFound) {
// TODO: should a missing record map to 404 tokenNotFound instead of 401 pinInvalid?
err = ErrPinInvalid
}
return nil, &GuestError{ErrorType: err, ShareID: shareID}
}
sessionToken, err := s.jwtService.Sign(shareID)
@@ -7,6 +7,8 @@ import (
"context"
"errors"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
@@ -307,13 +309,8 @@ func existingRecord() storage.Record {
}
func TestRenewPersistsRecordAndPublishesEvent(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(existingRecord(), nil)
var replaced storage.Record
store.On("Replace", mock.Anything).Run(func(args mock.Arguments) {
replaced = args.Get(0).(storage.Record)
}).Return(nil)
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(existingRecord()))
publisher := &testPublisher{}
gwc := newGatewayMock(&collaboration.GetShareResponse{
@@ -325,7 +322,8 @@ func TestRenewPersistsRecordAndPublishesEvent(t *testing.T) {
require.NoError(t, s.Renew(context.Background(), testShareID))
store.AssertCalled(t, "Replace", mock.Anything)
replaced, err := store.Get(token.Hash(testShareID))
require.NoError(t, err)
assert.Equal(t, testShareID, replaced.ShareID)
assert.Equal(t, token.Hash(testShareID), replaced.ShareIDHash)
assert.NotEmpty(t, replaced.SecretHash)
@@ -353,11 +351,13 @@ func TestRenewPersistsRecordAndPublishesEvent(t *testing.T) {
}
func TestRenewRecordNotFound(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(storage.Record{}, storage.ErrNotFound)
store := storage.NewFileManager(t.TempDir())
publisher := &testPublisher{}
s := newRenewService(t, store, newGatewayMock(&collaboration.GetShareResponse{}), publisher)
s := newRenewService(t, store, newGatewayMock(&collaboration.GetShareResponse{
Status: &rpc.Status{Code: rpc.Code_CODE_OK},
Share: newGuestShare(time.Now().Add(time.Hour)),
}), publisher)
err := s.Renew(context.Background(), testShareID)
require.Error(t, err)
@@ -367,25 +367,27 @@ func TestRenewRecordNotFound(t *testing.T) {
assert.ErrorIs(t, ge.ErrorType, storage.ErrNotFound)
assert.Equal(t, testShareID, ge.ShareID)
store.AssertNotCalled(t, "Replace", mock.Anything)
assert.Empty(t, publisher.published)
}
func TestRenewWithoutPublisher(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(existingRecord(), nil)
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(existingRecord()))
s := newRenewService(t, store, newGatewayMock(&collaboration.GetShareResponse{}), nil)
err := s.Renew(context.Background(), testShareID)
require.ErrorIs(t, err, ErrEventsNotConfigured)
store.AssertNotCalled(t, "Replace", mock.Anything)
got, err := store.Get(token.Hash(testShareID))
require.NoError(t, err)
assert.Empty(t, got.SecretHash)
assert.Empty(t, got.PinHash)
}
func TestRenewPublishFailure(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(existingRecord(), nil)
store.On("Replace", mock.Anything).Return(nil)
func TestRenewRollsBackOnPublishFailure(t *testing.T) {
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(existingRecord()))
publisher := &testPublisher{err: errors.New("publish failed")}
gwc := newGatewayMock(&collaboration.GetShareResponse{
@@ -398,11 +400,18 @@ func TestRenewPublishFailure(t *testing.T) {
err := s.Renew(context.Background(), testShareID)
require.Error(t, err)
assert.Contains(t, err.Error(), "publish failed")
got, err := store.Get(token.Hash(testShareID))
require.NoError(t, err)
assert.Empty(t, got.SecretHash)
assert.Empty(t, got.PinHash)
assert.True(t, got.PinExpiry.IsZero())
assert.False(t, got.Redeemed)
}
func TestRenewShareNotFound(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(existingRecord(), nil)
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(existingRecord()))
s := newRenewService(t, store, newGatewayMock(&collaboration.GetShareResponse{
Status: &rpc.Status{Code: rpc.Code_CODE_NOT_FOUND},
@@ -418,8 +427,8 @@ func TestRenewShareNotFound(t *testing.T) {
}
func TestRenewShareExpired(t *testing.T) {
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(existingRecord(), nil)
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(existingRecord()))
s := newRenewService(t, store, newGatewayMock(&collaboration.GetShareResponse{
Status: &rpc.Status{Code: rpc.Code_CODE_OK},
@@ -439,18 +448,13 @@ func TestVerifyPinSuccess(t *testing.T) {
pinHash, err := pin.Hash("123456")
require.NoError(t, err)
store := storagemocks.NewManager(t)
store.On("Get", token.Hash(testShareID)).Return(storage.Record{
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(storage.Record{
ShareID: testShareID,
ShareIDHash: token.Hash(testShareID),
PinHash: pinHash,
PinExpiry: time.Now().Add(time.Minute),
}, nil)
var replaced storage.Record
store.On("Replace", mock.Anything).Run(func(args mock.Arguments) {
replaced = args.Get(0).(storage.Record)
}).Return(nil)
}))
s := newRedeemService(t, store, newGatewayMock(&collaboration.GetShareResponse{
Status: &rpc.Status{Code: rpc.Code_CODE_OK},
@@ -462,9 +466,10 @@ func TestVerifyPinSuccess(t *testing.T) {
assert.NotEmpty(t, result.SessionToken)
assert.Equal(t, testShareID, result.ShareID)
store.AssertCalled(t, "Replace", mock.Anything)
assert.Empty(t, replaced.PinHash)
assert.True(t, replaced.PinExpiry.IsZero())
got, err := store.Get(token.Hash(testShareID))
require.NoError(t, err)
assert.Empty(t, got.PinHash)
assert.True(t, got.PinExpiry.IsZero())
}
func TestVerifyPinErrors(t *testing.T) {
@@ -494,7 +499,7 @@ func TestVerifyPinErrors(t *testing.T) {
name string
pin string
rec storage.Record
getErr error
missing bool
response *collaboration.GetShareResponse
wantErr error
}{
@@ -522,7 +527,7 @@ func TestVerifyPinErrors(t *testing.T) {
{
name: "record not found",
pin: "123456",
getErr: storage.ErrNotFound,
missing: true,
response: validShare,
wantErr: ErrPinInvalid,
},
@@ -549,11 +554,9 @@ func TestVerifyPinErrors(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
store := storagemocks.NewManager(t)
if tt.getErr != nil {
store.On("Get", token.Hash(testShareID)).Return(storage.Record{}, tt.getErr)
} else {
store.On("Get", token.Hash(testShareID)).Return(tt.rec, nil)
store := storage.NewFileManager(t.TempDir())
if !tt.missing {
require.NoError(t, store.Add(tt.rec))
}
s := newRedeemService(t, store, newGatewayMock(tt.response))
@@ -566,7 +569,49 @@ func TestVerifyPinErrors(t *testing.T) {
assert.ErrorIs(t, ge.ErrorType, tt.wantErr)
assert.Equal(t, testShareID, ge.ShareID)
store.AssertNotCalled(t, "Replace", mock.Anything)
if !tt.missing {
got, err := store.Get(token.Hash(testShareID))
require.NoError(t, err)
assert.Equal(t, tt.rec.PinHash, got.PinHash)
assert.WithinDuration(t, tt.rec.PinExpiry, got.PinExpiry, time.Second)
assert.Equal(t, tt.rec.Redeemed, got.Redeemed)
}
})
}
}
func TestVerifyPinSingleUseConcurrent(t *testing.T) {
pinHash, err := pin.Hash("123456")
require.NoError(t, err)
store := storage.NewFileManager(t.TempDir())
require.NoError(t, store.Add(storage.Record{
ShareID: testShareID,
ShareIDHash: token.Hash(testShareID),
PinHash: pinHash,
PinExpiry: time.Now().Add(time.Minute),
}))
s := newRedeemService(t, store, newGatewayMock(&collaboration.GetShareResponse{
Status: &rpc.Status{Code: rpc.Code_CODE_OK},
Share: newGuestShare(time.Now().Add(time.Hour)),
}))
const workers = 10
var (
wg sync.WaitGroup
success atomic.Int32
)
for range workers {
wg.Add(1)
go func() {
defer wg.Done()
if _, err := s.VerifyPin(context.Background(), testShareID, "123456"); err == nil {
success.Add(1)
}
}()
}
wg.Wait()
assert.Equal(t, int32(1), success.Load())
}
+3 -2
View File
@@ -7,6 +7,7 @@ import (
"crypto/rand"
"errors"
"fmt"
"math"
"math/big"
"github.com/alexedwards/argon2id"
@@ -18,12 +19,12 @@ var ErrInvalidPin = errors.New("invalid pin")
var ErrInvalidHash = errors.New("invalid pin hash")
func Generate() (string, error) {
n, err := rand.Int(rand.Reader, big.NewInt(1000000))
n, err := rand.Int(rand.Reader, big.NewInt(int64(math.Pow10(Length))))
if err != nil {
return "", fmt.Errorf("could not generate random pin: %w", err)
}
return fmt.Sprintf("%06d", n.Int64()), nil
return fmt.Sprintf("%0*d", Length, n.Int64()), nil
}
func Hash(p string) (string, error) {
@@ -53,17 +53,6 @@ func (s *FileManager) Add(rec Record) error {
return s.upsert(rec)
}
// Replace writes the record, overwriting an existing record for the same share id hash.
func (s *FileManager) Replace(rec Record) error {
lock, err := s.lockRecord(rec.ShareIDHash)
if err != nil {
return err
}
defer func() { _ = lock.Unlock() }()
return s.upsert(rec)
}
// Get returns the record for the given share id hash.
func (s *FileManager) Get(shareIDHash string) (Record, error) {
return s.get(shareIDHash)
@@ -114,6 +103,27 @@ func (s *FileManager) Redeem(shareIDHash string) error {
return s.upsert(rec)
}
// Update applies fn to the record under the record lock and writes it back.
// If fn returns an error the record is left unchanged.
func (s *FileManager) Update(shareIDHash string, fn func(*Record) error) error {
lock, err := s.lockRecord(shareIDHash)
if err != nil {
return err
}
defer func() { _ = lock.Unlock() }()
rec, err := s.get(shareIDHash)
if err != nil {
return err
}
if err := fn(&rec); err != nil {
return err
}
return s.upsert(rec)
}
func (s *FileManager) lockRecord(shareIDHash string) (*flock.Flock, error) {
p, err := s.path(shareIDHash)
if err != nil {
@@ -4,6 +4,7 @@
package storage
import (
"errors"
"io/fs"
"sync"
"sync/atomic"
@@ -39,36 +40,6 @@ func TestFileManagerAddGet(t *testing.T) {
assert.Equal(t, rec, got)
}
func TestFileManagerReplaceCreates(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
rec := newRecord("e0123456-7890-abcd-ef01-234567890abc")
require.NoError(t, s.Replace(rec))
got, err := s.Get(rec.ShareIDHash)
require.NoError(t, err)
assert.Equal(t, rec, got)
}
func TestFileManagerReplaceOverwrites(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
rec := newRecord("e0123456-7890-abcd-ef01-234567890abc")
require.NoError(t, s.Add(rec))
rec.SecretHash = "other"
rec.PinHash = "pinhash"
rec.PinExpiry = time.Date(2026, 12, 31, 23, 59, 59, 0, time.UTC)
rec.Redeemed = true
require.NoError(t, s.Replace(rec))
got, err := s.Get(rec.ShareIDHash)
require.NoError(t, err)
assert.Equal(t, rec, got)
}
func TestFileManagerGetMissing(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
@@ -100,7 +71,7 @@ func TestFileManagerInvalidHash(t *testing.T) {
require.ErrorIs(t, s.Remove("ab"), ErrInvalidHash)
require.ErrorIs(t, s.Add(Record{ShareIDHash: "ab"}), ErrInvalidHash)
require.ErrorIs(t, s.Replace(Record{ShareIDHash: "ab"}), ErrInvalidHash)
require.ErrorIs(t, s.Update("ab", func(*Record) error { return nil }), ErrInvalidHash)
}
func TestFileManagerRemove(t *testing.T) {
@@ -149,6 +120,52 @@ func TestFileManagerRedeemMissing(t *testing.T) {
assert.ErrorIs(t, err, ErrNotFound)
}
func TestFileManagerUpdate(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
rec := newRecord("e0123456-7890-abcd-ef01-234567890abc")
require.NoError(t, s.Add(rec))
require.NoError(t, s.Update(rec.ShareIDHash, func(r *Record) error {
r.PinHash = "pinhash"
r.Redeemed = true
return nil
}))
got, err := s.Get(rec.ShareIDHash)
require.NoError(t, err)
assert.Equal(t, "pinhash", got.PinHash)
assert.True(t, got.Redeemed)
}
func TestFileManagerUpdateErrorLeavesRecordUnchanged(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
rec := newRecord("e0123456-7890-abcd-ef01-234567890abc")
require.NoError(t, s.Add(rec))
errBoom := errors.New("boom")
err := s.Update(rec.ShareIDHash, func(r *Record) error {
r.PinHash = "pinhash"
return errBoom
})
assert.ErrorIs(t, err, errBoom)
got, err := s.Get(rec.ShareIDHash)
require.NoError(t, err)
assert.Equal(t, rec, got)
}
func TestFileManagerUpdateMissing(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
err := s.Update("doesnotexist", func(*Record) error { return nil })
assert.ErrorIs(t, err, ErrNotFound)
}
func TestFileManagerAddConcurrent(t *testing.T) {
dir := t.TempDir()
s := NewFileManager(dir)
@@ -249,53 +249,59 @@ func (_c *Manager_Remove_Call) RunAndReturn(run func(shareIDHash string) error)
return _c
}
// Replace provides a mock function for the type Manager
func (_mock *Manager) Replace(rec storage.Record) error {
ret := _mock.Called(rec)
// Update provides a mock function for the type Manager
func (_mock *Manager) Update(shareIDHash string, fn func(*storage.Record) error) error {
ret := _mock.Called(shareIDHash, fn)
if len(ret) == 0 {
panic("no return value specified for Replace")
panic("no return value specified for Update")
}
var r0 error
if returnFunc, ok := ret.Get(0).(func(storage.Record) error); ok {
r0 = returnFunc(rec)
if returnFunc, ok := ret.Get(0).(func(string, func(*storage.Record) error) error); ok {
r0 = returnFunc(shareIDHash, fn)
} else {
r0 = ret.Error(0)
}
return r0
}
// Manager_Replace_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Replace'
type Manager_Replace_Call struct {
// Manager_Update_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Update'
type Manager_Update_Call struct {
*mock.Call
}
// Replace is a helper method to define mock.On call
// - rec storage.Record
func (_e *Manager_Expecter) Replace(rec any) *Manager_Replace_Call {
return &Manager_Replace_Call{Call: _e.mock.On("Replace", rec)}
// Update is a helper method to define mock.On call
// - shareIDHash string
// - fn func(*storage.Record) error
func (_e *Manager_Expecter) Update(shareIDHash any, fn any) *Manager_Update_Call {
return &Manager_Update_Call{Call: _e.mock.On("Update", shareIDHash, fn)}
}
func (_c *Manager_Replace_Call) Run(run func(rec storage.Record)) *Manager_Replace_Call {
func (_c *Manager_Update_Call) Run(run func(shareIDHash string, fn func(*storage.Record) error)) *Manager_Update_Call {
_c.Call.Run(func(args mock.Arguments) {
var arg0 storage.Record
var arg0 string
if args[0] != nil {
arg0 = args[0].(storage.Record)
arg0 = args[0].(string)
}
var arg1 func(*storage.Record) error
if args[1] != nil {
arg1 = args[1].(func(*storage.Record) error)
}
run(
arg0,
arg1,
)
})
return _c
}
func (_c *Manager_Replace_Call) Return(err error) *Manager_Replace_Call {
func (_c *Manager_Update_Call) Return(err error) *Manager_Update_Call {
_c.Call.Return(err)
return _c
}
func (_c *Manager_Replace_Call) RunAndReturn(run func(rec storage.Record) error) *Manager_Replace_Call {
func (_c *Manager_Update_Call) RunAndReturn(run func(shareIDHash string, fn func(*storage.Record) error) error) *Manager_Update_Call {
_c.Call.Return(run)
return _c
}
@@ -25,8 +25,8 @@ type Record struct {
type Manager interface {
Add(rec Record) error
Replace(rec Record) error
Get(shareIDHash string) (Record, error)
Remove(shareIDHash string) error
Redeem(shareIDHash string) error
Update(shareIDHash string, fn func(*Record) error) error
}