diff --git a/core/authenticate/mocks/consent_service.go b/core/authenticate/mocks/consent_service.go index 52fe63f60..692614702 100644 --- a/core/authenticate/mocks/consent_service.go +++ b/core/authenticate/mocks/consent_service.go @@ -116,6 +116,64 @@ func (_c *ConsentService_RecordGranted_Call) RunAndReturn(run func(context.Conte return _c } +// Resolve provides a mock function with given fields: ids +func (_m *ConsentService) Resolve(ids []string) ([]consent.Document, error) { + ret := _m.Called(ids) + + if len(ret) == 0 { + panic("no return value specified for Resolve") + } + + var r0 []consent.Document + var r1 error + if rf, ok := ret.Get(0).(func([]string) ([]consent.Document, error)); ok { + return rf(ids) + } + if rf, ok := ret.Get(0).(func([]string) []consent.Document); ok { + r0 = rf(ids) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]consent.Document) + } + } + + if rf, ok := ret.Get(1).(func([]string) error); ok { + r1 = rf(ids) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ConsentService_Resolve_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Resolve' +type ConsentService_Resolve_Call struct { + *mock.Call +} + +// Resolve is a helper method to define mock.On call +// - ids []string +func (_e *ConsentService_Expecter) Resolve(ids interface{}) *ConsentService_Resolve_Call { + return &ConsentService_Resolve_Call{Call: _e.mock.On("Resolve", ids)} +} + +func (_c *ConsentService_Resolve_Call) Run(run func(ids []string)) *ConsentService_Resolve_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].([]string)) + }) + return _c +} + +func (_c *ConsentService_Resolve_Call) Return(_a0 []consent.Document, _a1 error) *ConsentService_Resolve_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *ConsentService_Resolve_Call) RunAndReturn(run func([]string) ([]consent.Document, error)) *ConsentService_Resolve_Call { + _c.Call.Return(run) + return _c +} + // ResolveAll provides a mock function with given fields: ids func (_m *ConsentService) ResolveAll(ids []string) ([]consent.Document, error) { ret := _m.Called(ids) diff --git a/core/authenticate/service.go b/core/authenticate/service.go index 4273023fa..b7740e9ee 100644 --- a/core/authenticate/service.go +++ b/core/authenticate/service.go @@ -77,9 +77,11 @@ type UserService interface { } // ConsentService checks what a caller accepted against what the deployment -// configures, and writes the record for it. With app.consent disabled it -// resolves nothing, so an empty document set is the signal to write no record. +// configures, and writes the record for it. With app.consent disabled both +// Resolve and ResolveAll resolve nothing, so an empty document set is the signal +// to write no record, and the ids are ignored rather than rejected. type ConsentService interface { + Resolve(ids []string) ([]consent.Document, error) ResolveAll(ids []string) ([]consent.Document, error) Grant(ctx context.Context, tx *sqlx.Tx, req consent.GrantRequest) (consent.Consent, error) RecordGranted(ctx context.Context, granted consent.Consent) @@ -231,6 +233,11 @@ func (s Service) StartFlow(ctx context.Context, request RegistrationStartRequest if !utils.Contains(s.SupportedStrategies(), request.Method) { return nil, ErrUnsupportedMethod } + // the consent gate runs first: a check on the request alone, costing no + // lookup, and it has to fail before anything is sent or redirected + if err := s.gateFlowConsent(request.Intent, request.AcceptedDocumentIDs); err != nil { + return nil, err + } // both mail strategies know the address before anything is sent, and share // applyMailOTP at the other end, so they share the gate here. Passkey gates // in its own branch, where it already looks the user up to pick a ceremony. @@ -459,6 +466,30 @@ func (s Service) gateFlowStart(ctx context.Context, intent FlowIntent, email str return checkIntent(intent, err == nil) } +// gateFlowConsent is the consent half of the flow start check, and the intent +// decides which rule applies, because without one a signup and a login look +// identical. A signup runs the completeness rule here, before anything is sent +// or redirected — true for OIDC too, where the email is unknown but the intent +// is not. Without an intent only the unknown-id rule runs, and completeness +// waits for user creation. A login checks nothing, because it writes no record. +func (s Service) gateFlowConsent(intent FlowIntent, ids []string) error { + if s.consentService == nil || intent == FlowIntentLogin { + return nil + } + + var err error + if intent == FlowIntentSignup { + _, err = s.consentService.ResolveAll(ids) + } else { + _, err = s.consentService.Resolve(ids) + } + if err != nil { + // the wrapped error names what is missing, for the log not the response + return fmt.Errorf("%w: %w", ErrConsentRequired, err) + } + return nil +} + // applyMailOTP actions when user submitted otp from the email // user can be considered as verified if code is valid // create a new user if required diff --git a/core/authenticate/service_test.go b/core/authenticate/service_test.go index 9d27cc6ad..0b0c784f1 100644 --- a/core/authenticate/service_test.go +++ b/core/authenticate/service_test.go @@ -1351,6 +1351,162 @@ func TestService_StartFlow_WritesIntentAndConsent(t *testing.T) { }) } +// TestService_StartFlow_Consent covers the first of the two consent gates. It +// is the one that exists for the error: it runs before an OTP is sent and +// before the browser leaves for an identity provider, so a rejection costs the +// user a retry and nothing else. +// +// The intent decides which rule applies, because without one a signup and a +// login look identical. +func TestService_StartFlow_Consent(t *testing.T) { + defaultHashCost := authenticate.OTPHashCost + authenticate.OTPHashCost = bcrypt.MinCost + t.Cleanup(func() { authenticate.OTPHashCost = defaultHashCost }) + + const email = "test@example.com" + documents := []consent.Document{ + {ID: "privacy_policy", Title: "Privacy Policy", Version: "2026-04-01", URL: "https://example.org/p"}, + {ID: "terms_of_service", Title: "Terms & Conditions", Version: "2026-04-01", URL: "https://example.org/t"}, + } + acceptedIDs := []string{"privacy_policy", "terms_of_service"} + + // startFlow runs a mail otp flow start against whatever consent service it + // is given. Mail otp is the strategy that shows the point of this gate, + // because a rejection here is a code that never gets sent. The dialer is a + // bare mock with no expectations, so a flow that reaches SendMail fails + // rather than passing quietly. + startFlow := func(t *testing.T, consentService authenticate.ConsentService, + request authenticate.RegistrationStartRequest, wantFlow bool) (*authenticate.RegistrationStartResponse, error) { + t.Helper() + + ctx := context.Background() + mockFlowRepo, mockUserService, _, _, _ := createMocks(t) + if request.Intent != authenticate.FlowIntentUnspecified { + mockUserService.EXPECT().GetByID(ctx, email). + Return(user.User{}, errors.New("user not found")).Maybe() + } + + var dialer mailer.Dialer = &mailerMock.Dialer{} + if wantFlow { + mockFlowRepo.EXPECT().Set(ctx, mock.Anything).Return(nil) + dialer = mailer.NewMockDialer() + } + + srv := authenticate.NewService(nil, authenticate.Config{ + MailOTP: authenticate.MailOTPConfig{Validity: 10 * time.Minute}, + TestUsers: testusers.Config{Enabled: true, OTP: "111111", Domain: "example.com"}, + }, mockFlowRepo, dialer, nil, nil, mockUserService, nil, nil, nil, consentService, nil) + + request.Method = authenticate.MailOTPAuthMethod.String() + request.Email = email + return srv.StartFlow(ctx, request) + } + + t.Run("a signup carrying every document starts the flow", func(t *testing.T) { + mockConsent := mocks.NewConsentService(t) + mockConsent.EXPECT().ResolveAll(acceptedIDs).Return(documents, nil) + + got, err := startFlow(t, mockConsent, authenticate.RegistrationStartRequest{ + Intent: authenticate.FlowIntentSignup, + AcceptedDocumentIDs: acceptedIDs, + }, true) + + require.NoError(t, err) + require.NotNil(t, got) + }) + + t.Run("a signup missing a document is rejected before the code is sent", func(t *testing.T) { + mockConsent := mocks.NewConsentService(t) + mockConsent.EXPECT().ResolveAll([]string{"privacy_policy"}). + Return(nil, consent.ErrMissingDocuments) + + got, err := startFlow(t, mockConsent, authenticate.RegistrationStartRequest{ + Intent: authenticate.FlowIntentSignup, + AcceptedDocumentIDs: []string{"privacy_policy"}, + }, false) + + assert.ErrorIs(t, err, authenticate.ErrConsentRequired) + // the wrapped error still names what was missing, for the log + assert.ErrorIs(t, err, consent.ErrMissingDocuments) + assert.Nil(t, got) + }) + + t.Run("a signup carrying no document at all is rejected too", func(t *testing.T) { + mockConsent := mocks.NewConsentService(t) + mockConsent.EXPECT().ResolveAll([]string(nil)).Return(nil, consent.ErrMissingDocuments) + + _, err := startFlow(t, mockConsent, authenticate.RegistrationStartRequest{ + Intent: authenticate.FlowIntentSignup, + }, false) + + assert.ErrorIs(t, err, authenticate.ErrConsentRequired) + }) + + t.Run("an unspecified intent checks only that the ids are known", func(t *testing.T) { + // frontier cannot yet know this request will create a user, so + // completeness waits for user creation, but a typo can still be caught + // before the redirect + mockConsent := mocks.NewConsentService(t) + mockConsent.EXPECT().Resolve([]string{"privacy_policy"}).Return(documents[:1], nil) + + _, err := startFlow(t, mockConsent, authenticate.RegistrationStartRequest{ + AcceptedDocumentIDs: []string{"privacy_policy"}, + }, true) + + require.NoError(t, err) + }) + + t.Run("an unspecified intent is rejected for an id config does not know", func(t *testing.T) { + mockConsent := mocks.NewConsentService(t) + mockConsent.EXPECT().Resolve([]string{"not_a_document"}). + Return(nil, consent.ErrUnknownDocuments) + + _, err := startFlow(t, mockConsent, authenticate.RegistrationStartRequest{ + AcceptedDocumentIDs: []string{"not_a_document"}, + }, false) + + assert.ErrorIs(t, err, authenticate.ErrConsentRequired) + assert.ErrorIs(t, err, consent.ErrUnknownDocuments) + }) + + t.Run("a login checks nothing, because it writes no record", func(t *testing.T) { + // an unexpected call fails the test rather than passing silently + mockConsent := mocks.NewConsentService(t) + + mockFlowRepo, mockUserService, _, _, _ := createMocks(t) + ctx := context.Background() + mockUserService.EXPECT().GetByID(ctx, email).Return(user.User{ID: "user-id", Email: email}, nil) + mockFlowRepo.EXPECT().Set(ctx, mock.Anything).Return(nil) + + srv := authenticate.NewService(nil, authenticate.Config{ + MailOTP: authenticate.MailOTPConfig{Validity: 10 * time.Minute}, + TestUsers: testusers.Config{Enabled: true, OTP: "111111", Domain: "example.com"}, + }, mockFlowRepo, mailer.NewMockDialer(), nil, nil, mockUserService, nil, nil, nil, mockConsent, nil) + + _, err := srv.StartFlow(ctx, authenticate.RegistrationStartRequest{ + Method: authenticate.MailOTPAuthMethod.String(), + Email: email, + Intent: authenticate.FlowIntentLogin, + }) + require.NoError(t, err) + }) + + t.Run("a deployment with consent disabled ignores the ids rather than rejecting them", func(t *testing.T) { + // one client build works against both kinds of deployment, so the same + // signup that a configured deployment gates goes straight through here + disabled := consent.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), + consent.Config{Enabled: false}, nil, nil) + + got, err := startFlow(t, disabled, authenticate.RegistrationStartRequest{ + Intent: authenticate.FlowIntentSignup, + AcceptedDocumentIDs: acceptedIDs, + }, true) + + require.NoError(t, err) + require.NotNil(t, got) + }) +} + // TestFlow_IntentAndConsent covers the accessors directly: the JSON round trip // the database puts metadata through, and the nil receiver callers rely on. func TestFlow_IntentAndConsent(t *testing.T) { @@ -1499,6 +1655,8 @@ func TestService_FinishFlow_Intent(t *testing.T) { if tt.wantErr != nil { assert.ErrorIs(t, err, tt.wantErr) + // a rejection is the error and nothing else: the handler maps it + // to a connect code and the caller decides what to do with it assert.Nil(t, got) return } @@ -1623,6 +1781,7 @@ func TestService_FinishFlow_Consent(t *testing.T) { assert.ErrorIs(t, err, authenticate.ErrConsentRequired) // the wrapped error still names what was missing assert.ErrorIs(t, err, consent.ErrMissingDocuments) + // the rejection is the error and nothing else assert.Nil(t, got) mockUserService.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) mockUserService.AssertNotCalled(t, "CreateWithTx", mock.Anything, mock.Anything, mock.Anything) diff --git a/internal/api/v1beta1connect/authenticate.go b/internal/api/v1beta1connect/authenticate.go index 738f978b5..b3d2f1648 100644 --- a/internal/api/v1beta1connect/authenticate.go +++ b/internal/api/v1beta1connect/authenticate.go @@ -25,7 +25,58 @@ import ( "google.golang.org/protobuf/types/known/structpb" ) +// authFlowRejection carries a gate or consent error out to a client. Both auth +// RPCs answer with a connect code, because both are called from application +// JavaScript — AuthCallback from whatever page the callback URL points at — so +// neither is a redirect the browser follows on its own. +type authFlowRejection struct { + code connect.Code + // err is the bare sentinel; the wrapped one names the documents and belongs + // in a log, not a response. + err error +} + +// lookupAuthFlowRejection maps the three errors both auth RPCs have to make +// legible. FailedPrecondition for consent, because it is what separates a +// consent rejection from a bad code or an expired flow, which are +// InvalidArgument. The codes are distinct, so a client needs no second +// vocabulary alongside them. Anything missing here surfaces as a 500. +func lookupAuthFlowRejection(err error) (authFlowRejection, bool) { + switch { + case errors.Is(err, authenticate.ErrLoginUserNotFound): + return authFlowRejection{ + code: connect.CodeNotFound, + err: authenticate.ErrLoginUserNotFound, + }, true + case errors.Is(err, authenticate.ErrSignupUserExists): + return authFlowRejection{ + code: connect.CodeAlreadyExists, + err: authenticate.ErrSignupUserExists, + }, true + case errors.Is(err, authenticate.ErrConsentRequired): + return authFlowRejection{ + code: connect.CodeFailedPrecondition, + err: authenticate.ErrConsentRequired, + }, true + } + return authFlowRejection{}, false +} + +// toFlowIntent maps the request enum onto the flow intent. An unknown value +// reads as unspecified, which is the behaviour clients had before intents. +func toFlowIntent(intent frontierv1beta1.FlowIntent) authenticate.FlowIntent { + switch intent { + case frontierv1beta1.FlowIntent_FLOW_INTENT_LOGIN: + return authenticate.FlowIntentLogin + case frontierv1beta1.FlowIntent_FLOW_INTENT_SIGNUP: + return authenticate.FlowIntentSignup + default: + return authenticate.FlowIntentUnspecified + } +} + func (h *ConnectHandler) Authenticate(ctx context.Context, request *connect.Request[frontierv1beta1.AuthenticateRequest]) (*connect.Response[frontierv1beta1.AuthenticateResponse], error) { + errorLogger := NewErrorLogger() returnToURL := h.authnService.SanitizeReturnToURL(request.Msg.GetReturnTo()) callbackURL := h.authnService.SanitizeCallbackURL(request.Msg.GetCallbackUrl()) @@ -46,14 +97,36 @@ func (h *ConnectHandler) Authenticate(ctx context.Context, request *connect.Requ return nil, connect.NewError(connect.CodeInvalidArgument, ErrInvalidEmail) } + intent := toFlowIntent(request.Msg.GetFlowIntent()) + acceptedDocumentIDs := request.Msg.GetAcceptedDocumentIds() + if intent == authenticate.FlowIntentLogin && len(acceptedDocumentIDs) > 0 { + // a login writes no record, so accepting these silently would leave the + // client believing it recorded a consent that does not exist + return nil, connect.NewError(connect.CodeInvalidArgument, ErrConsentOnLoginIntent) + } + + // Authenticate is skip-listed, so nothing put session metadata on the context + // and the handler extracts it itself. Only the IP is passed on. + sessionMetadata := sessionutils.ExtractSessionMetadata(ctx, request, h.authConfig.Session.Headers) + // not logged in, try registration response, err := h.authnService.StartFlow(ctx, authenticate.RegistrationStartRequest{ - Method: request.Msg.GetStrategyName(), - ReturnToURL: returnToURL, - CallbackUrl: callbackURL, - Email: request.Msg.GetEmail(), + Method: request.Msg.GetStrategyName(), + ReturnToURL: returnToURL, + CallbackUrl: callbackURL, + Email: request.Msg.GetEmail(), + Intent: intent, + AcceptedDocumentIDs: acceptedDocumentIDs, + IPAddress: sessionMetadata.IpAddress, }) if err != nil { + // their own codes rather than a 500; the wrapped error stays in the log + if rejection, ok := lookupAuthFlowRejection(err); ok { + errorLogger.LogServiceError(ctx, request, "Authenticate.StartFlow", err, + "strategy", request.Msg.GetStrategyName(), + "intent", intent.String()) + return nil, connect.NewError(rejection.code, rejection.err) + } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("Authenticate: strategy=%s email=%s: %w", request.Msg.GetStrategyName(), request.Msg.GetEmail(), err)) } @@ -107,6 +180,18 @@ func (h *ConnectHandler) AuthCallback(ctx context.Context, request *connect.Requ StateConfig: request.Msg.GetStateOptions().AsMap(), }) if err != nil { + // These join the errors handled here rather than falling through to + // Internal, keeping their own codes rather than this list's + // InvalidArgument. The rejection is the answer and not a redirect: the + // callback URL points at a page the application hosts, and that page is + // what calls this RPC, so it decides where the user goes next. + if rejection, ok := lookupAuthFlowRejection(err); ok { + errorLogger.LogServiceError(ctx, request, "AuthCallback.FinishFlow", err, + "strategy", request.Msg.GetStrategyName(), + "state", request.Msg.GetState()) + return nil, connect.NewError(rejection.code, rejection.err) + } + // ErrUnsupportedMethod here means the strategy and state the client sent match // no known method (e.g. a malformed, non-base64 state). That is bad client // input, same class as an empty or invalid state, so return a 4xx not a 500. diff --git a/internal/api/v1beta1connect/authenticate_test.go b/internal/api/v1beta1connect/authenticate_test.go index 3e4bc251f..42fd80a53 100644 --- a/internal/api/v1beta1connect/authenticate_test.go +++ b/internal/api/v1beta1connect/authenticate_test.go @@ -3,11 +3,15 @@ package v1beta1connect import ( "context" "encoding/json" + "errors" + "fmt" "testing" "connectrpc.com/connect" "github.com/lestrrat-go/jwx/v2/jwk" "github.com/raystack/frontier/core/authenticate" + frontiersession "github.com/raystack/frontier/core/authenticate/session" + "github.com/raystack/frontier/core/consent" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/serviceuser" "github.com/raystack/frontier/internal/api/v1beta1connect/mocks" @@ -333,3 +337,248 @@ func TestToJSONWebKey(t *testing.T) { }) } } + +// testSessionHeaders is the default header set, so ExtractSessionMetadata reads +// the same headers the server reads. +func testSessionHeaders() authenticate.Config { + return authenticate.Config{ + Session: authenticate.SessionConfig{ + Headers: authenticate.SessionMetadataHeaders{ + ClientIP: "x-forwarded-for", + ClientUserAgent: "User-Agent", + }, + }, + } +} + +// TestConnectHandler_Authenticate_PassesIntentConsentAndIP pins what the handler +// hands StartFlow. Authenticate is on the authentication skip list, so nothing +// puts session metadata on the context and the handler extracts it itself. +func TestConnectHandler_Authenticate_PassesIntentConsentAndIP(t *testing.T) { + ctx := context.Background() + + mockAuthnSrv := mocks.NewAuthnService(t) + mockSessionSrv := mocks.NewSessionService(t) + + mockAuthnSrv.EXPECT().SanitizeReturnToURL("https://example.org/done").Return("https://example.org/done") + mockAuthnSrv.EXPECT().SanitizeCallbackURL("").Return("https://example.org/callback") + mockSessionSrv.EXPECT().ExtractFromContext(ctx).Return(nil, frontiersession.ErrNoSession) + + var startRequest authenticate.RegistrationStartRequest + mockAuthnSrv.EXPECT().StartFlow(ctx, mock.Anything). + Run(func(_ context.Context, req authenticate.RegistrationStartRequest) { startRequest = req }). + Return(&authenticate.RegistrationStartResponse{Flow: &authenticate.Flow{}}, nil) + + handler := &ConnectHandler{ + authnService: mockAuthnSrv, + sessionService: mockSessionSrv, + authConfig: testSessionHeaders(), + } + + request := connect.NewRequest(&frontierv1beta1.AuthenticateRequest{ + StrategyName: authenticate.MailOTPAuthMethod.String(), + Email: "test@example.com", + ReturnTo: "https://example.org/done", + FlowIntent: frontierv1beta1.FlowIntent_FLOW_INTENT_SIGNUP, + AcceptedDocumentIds: []string{"terms_of_service", "privacy_policy"}, + }) + request.Header().Set("x-forwarded-for", "203.0.113.9, 10.0.0.1") + request.Header().Set("User-Agent", "Mozilla/5.0 (Macintosh) Chrome/124.0") + + _, err := handler.Authenticate(ctx, request) + require.NoError(t, err) + + assert.Equal(t, authenticate.FlowIntentSignup, startRequest.Intent) + assert.Equal(t, []string{"terms_of_service", "privacy_policy"}, startRequest.AcceptedDocumentIDs) + // the first hop of the forwarded chain reaches the consent record, and the + // user agent does not: the helper keeps an OS and a browser family from it + // and drops the raw string, and neither is passed on + assert.Equal(t, "203.0.113.9", startRequest.IPAddress) +} + +// TestConnectHandler_Authenticate_RejectsIdsWithALoginIntent covers the one +// request shape the handler turns down on its own. A login writes no consent +// record, so accepting the ids would leave the client believing it recorded a +// consent that does not exist. +func TestConnectHandler_Authenticate_RejectsIdsWithALoginIntent(t *testing.T) { + ctx := context.Background() + + mockAuthnSrv := mocks.NewAuthnService(t) + mockSessionSrv := mocks.NewSessionService(t) + mockAuthnSrv.EXPECT().SanitizeReturnToURL("").Return("") + mockAuthnSrv.EXPECT().SanitizeCallbackURL("").Return("https://example.org/callback") + mockSessionSrv.EXPECT().ExtractFromContext(ctx).Return(nil, frontiersession.ErrNoSession) + + handler := &ConnectHandler{ + authnService: mockAuthnSrv, + sessionService: mockSessionSrv, + authConfig: testSessionHeaders(), + } + + resp, err := handler.Authenticate(ctx, connect.NewRequest(&frontierv1beta1.AuthenticateRequest{ + StrategyName: authenticate.MailOTPAuthMethod.String(), + Email: "test@example.com", + FlowIntent: frontierv1beta1.FlowIntent_FLOW_INTENT_LOGIN, + AcceptedDocumentIds: []string{"terms_of_service"}, + })) + + assert.Nil(t, resp) + connectErr := err.(*connect.Error) + assert.Equal(t, connect.CodeInvalidArgument, connectErr.Code()) + assert.Equal(t, ErrConsentOnLoginIntent.Error(), connectErr.Message()) + // the flow is never started for a request the handler will not accept + mockAuthnSrv.AssertNotCalled(t, "StartFlow", mock.Anything, mock.Anything) +} + +// TestConnectHandler_Authenticate_Rejections covers the three errors reaching +// the client with their own codes. Authenticate is an XHR, so it answers with +// the code and no redirect. +func TestConnectHandler_Authenticate_Rejections(t *testing.T) { + tests := []struct { + name string + err error + wantCode connect.Code + wantMsg string + }{ + { + name: "a login for an address with no account is a not found", + err: authenticate.ErrLoginUserNotFound, + wantCode: connect.CodeNotFound, + wantMsg: authenticate.ErrLoginUserNotFound.Error(), + }, + { + name: "a signup for an address that has one already exists", + err: authenticate.ErrSignupUserExists, + wantCode: connect.CodeAlreadyExists, + wantMsg: authenticate.ErrSignupUserExists.Error(), + }, + { + // FailedPrecondition is what lets a client separate a consent + // rejection from a bad code or an expired flow + name: "an incomplete consent is a failed precondition", + err: fmt.Errorf("%w: %w: terms_of_service", authenticate.ErrConsentRequired, consent.ErrMissingDocuments), + wantCode: connect.CodeFailedPrecondition, + wantMsg: authenticate.ErrConsentRequired.Error(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + mockAuthnSrv := mocks.NewAuthnService(t) + mockSessionSrv := mocks.NewSessionService(t) + mockAuthnSrv.EXPECT().SanitizeReturnToURL("").Return("") + mockAuthnSrv.EXPECT().SanitizeCallbackURL("").Return("https://example.org/callback") + mockSessionSrv.EXPECT().ExtractFromContext(ctx).Return(nil, frontiersession.ErrNoSession) + mockAuthnSrv.EXPECT().StartFlow(ctx, mock.Anything).Return(nil, tt.err) + + handler := &ConnectHandler{ + authnService: mockAuthnSrv, + sessionService: mockSessionSrv, + authConfig: testSessionHeaders(), + } + + resp, err := handler.Authenticate(ctx, connect.NewRequest(&frontierv1beta1.AuthenticateRequest{ + StrategyName: authenticate.MailOTPAuthMethod.String(), + Email: "test@example.com", + })) + + assert.Nil(t, resp) + connectErr := err.(*connect.Error) + assert.Equal(t, tt.wantCode, connectErr.Code()) + // the bare sentinel, never the wrapped error: which documents were + // missing belongs in a log, not in the response + assert.Equal(t, tt.wantMsg, connectErr.Message()) + assert.NotContains(t, connectErr.Message(), "terms_of_service") + }) + } +} + +// TestConnectHandler_AuthCallback_Rejections covers the settled decision: the +// callback is a browser navigation, so a rejection sends the user back to the +// page they started from with a machine-readable code, not onto an error page. +func TestConnectHandler_AuthCallback_Rejections(t *testing.T) { + tests := []struct { + name string + err error + + wantCode connect.Code + wantMsg string + }{ + { + name: "a login for an address with no account", + err: authenticate.ErrLoginUserNotFound, + wantCode: connect.CodeNotFound, + wantMsg: authenticate.ErrLoginUserNotFound.Error(), + }, + { + name: "a signup for an address that has one", + err: authenticate.ErrSignupUserExists, + wantCode: connect.CodeAlreadyExists, + wantMsg: authenticate.ErrSignupUserExists.Error(), + }, + { + name: "an incomplete consent", + err: fmt.Errorf("%w: %w: terms_of_service", authenticate.ErrConsentRequired, consent.ErrMissingDocuments), + wantCode: connect.CodeFailedPrecondition, + wantMsg: authenticate.ErrConsentRequired.Error(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + mockAuthnSrv := mocks.NewAuthnService(t) + mockAuthnSrv.EXPECT().FinishFlow(ctx, mock.Anything).Return(nil, tt.err) + + handler := &ConnectHandler{authnService: mockAuthnSrv} + + resp, err := handler.AuthCallback(ctx, connect.NewRequest(&frontierv1beta1.AuthCallbackRequest{ + StrategyName: authenticate.MailOTPAuthMethod.String(), + State: "state", + Code: "111111", + })) + + // the rejection is the answer. The callback URL points at a page the + // application hosts, and that page is what called this RPC, so it + // has the code in hand and decides where the user goes next. + assert.Nil(t, resp) + connectErr := err.(*connect.Error) + assert.Equal(t, tt.wantCode, connectErr.Code()) + // the bare sentinel: the wrapped error names the documents that were + // missing, and that belongs in the log rather than in the response + assert.Equal(t, tt.wantMsg, connectErr.Message()) + assert.NotContains(t, connectErr.Message(), "terms_of_service") + }) + } +} + +// TestConnectHandler_AuthCallback_UnmappedErrorStaysInternal pins the boundary +// of the closed code set: an error with no code of its own is not turned into a +// redirect with some passthrough string. +func TestConnectHandler_AuthCallback_UnmappedErrorStaysInternal(t *testing.T) { + ctx := context.Background() + + mockAuthnSrv := mocks.NewAuthnService(t) + mockAuthnSrv.EXPECT().FinishFlow(ctx, mock.Anything).Return(nil, errors.New("database is down")) + + handler := &ConnectHandler{authnService: mockAuthnSrv} + resp, err := handler.AuthCallback(ctx, connect.NewRequest(&frontierv1beta1.AuthCallbackRequest{ + StrategyName: authenticate.MailOTPAuthMethod.String(), + State: "state", + })) + + assert.Nil(t, resp) + assert.Equal(t, connect.CodeInternal, err.(*connect.Error).Code()) +} + +func TestToFlowIntent(t *testing.T) { + assert.Equal(t, authenticate.FlowIntentLogin, toFlowIntent(frontierv1beta1.FlowIntent_FLOW_INTENT_LOGIN)) + assert.Equal(t, authenticate.FlowIntentSignup, toFlowIntent(frontierv1beta1.FlowIntent_FLOW_INTENT_SIGNUP)) + assert.Equal(t, authenticate.FlowIntentUnspecified, toFlowIntent(frontierv1beta1.FlowIntent_FLOW_INTENT_UNSPECIFIED)) + // an unknown value reads as unspecified, which is the create-or-get + // behaviour every client had before intents existed + assert.Equal(t, authenticate.FlowIntentUnspecified, toFlowIntent(frontierv1beta1.FlowIntent(99))) +} diff --git a/internal/api/v1beta1connect/errors.go b/internal/api/v1beta1connect/errors.go index f8eddc17a..4924bd94a 100644 --- a/internal/api/v1beta1connect/errors.go +++ b/internal/api/v1beta1connect/errors.go @@ -73,4 +73,5 @@ var ( ErrInvalidSessionID = errors.New("invalid session_id format: must be a valid UUID") ErrInvalidUserID = errors.New("invalid user_id format: must be a valid UUID") ErrRoleNotFound = errors.New("role doesn't exist") + ErrConsentOnLoginIntent = errors.New("accepted_document_ids can only be sent with a signup intent") )