From 8e30f1a180248c42f41192a33aa40c27bdcfd876 Mon Sep 17 00:00:00 2001 From: Rohan Chakraborty Date: Sun, 30 Aug 2026 20:51:13 +0530 Subject: [PATCH] feat(authenticate): map the gate and consent errors on both auth RPCs The two request fields reach StartFlow, and the three rejections become legible to a client instead of arriving as a 500. Authenticate calls sessionutils.ExtractSessionMetadata itself for the IP. Authenticate and AuthCallback are both on the authentication skip list, so nothing puts session metadata on the context. That helper parses the user agent into an OS and a browser family and drops the raw string, so what reaches the consent record is the IP and nothing else, from when the user accepted rather than from the callback. ErrLoginUserNotFound maps to NotFound, ErrSignupUserExists to AlreadyExists and ErrConsentRequired to FailedPrecondition, from both RPCs. All three had to join the errors AuthCallback handles explicitly, which maps a fixed list to a 4xx and everything else to Internal; they keep their own codes rather than the InvalidArgument the rest of that list gets, because FailedPrecondition is what lets a client separate a consent rejection from a bad code or an expired flow. Both RPCs answer with a code rather than a redirect. Frontier serves no route for the callback URL: it points at a page the application hosts, and that page is what calls AuthCallback over connect, so it already holds the rejection and decides where the user goes next. Handing it a location header instead would oblige every such page to be written to read one, and a 2xx carrying that header reads as success to one that is not. The three codes are distinct, so a client tells the rejections apart without a second vocabulary alongside them. StartFlow gains the consent half of the flow start gate, so a rejection lands before an OTP is sent and before the browser leaves for an identity provider. A signup intent runs ResolveAll there; an unspecified intent runs Resolve, which still catches an unknown id before the redirect while completeness waits for user creation; a login intent checks nothing, because it writes no record. Ids sent with a login intent are rejected by the handler as InvalidArgument, since accepting them silently would leave a client believing it recorded a consent that does not exist. With app.consent disabled both Resolve and ResolveAll resolve nothing and reject nothing, so the ids are ignored rather than rejected and one client build works against both kinds of deployment. Rendering any of this is a separate frontend change: the sign-in and sign-up views for what Authenticate returns, and the callback page for what AuthCallback returns. Refs docs/rfcs/0002-explicit-consent-at-signup.md, Enforcement. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Hf1XuPCWcHZ7QY5u4WBB2G --- core/authenticate/mocks/consent_service.go | 58 ++++ core/authenticate/service.go | 35 ++- core/authenticate/service_test.go | 159 +++++++++++ internal/api/v1beta1connect/authenticate.go | 93 ++++++- .../api/v1beta1connect/authenticate_test.go | 249 ++++++++++++++++++ internal/api/v1beta1connect/errors.go | 1 + 6 files changed, 589 insertions(+), 6 deletions(-) diff --git a/core/authenticate/mocks/consent_service.go b/core/authenticate/mocks/consent_service.go index 52fe63f60c..692614702a 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 4273023fa7..b7740e9ee1 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 9d27cc6ad9..0b0c784f15 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 738f978b54..b3d2f1648c 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 3e4bc251fa..42fd80a536 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 f8eddc17aa..4924bd94ad 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") )