diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 5a0926080..ca1be6ccc 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -439,7 +439,8 @@ const ( ResetTokenStateValid ResetTokenState = "valid" // ResetTokenStateExpired means the token matches but its expiry has passed. ResetTokenStateExpired ResetTokenState = "expired" - // ResetTokenStateUsed covers both consumed and never-issued tokens. + // ResetTokenStateUsed covers consumed, never-issued, and + // deactivated-account tokens. ResetTokenStateUsed ResetTokenState = "used" ) @@ -497,6 +498,13 @@ func (s *Service) ResetTokenStatus(ctx context.Context, token string) (ResetToke return ResetTokenStateExpired, resetTokenFlowFor(user), nil } + // ConfirmPasswordReset refuses admin-deactivated accounts, so report the + // token as used (terminal view, no form that can never succeed). Using + // "used" keeps the endpoint from revealing that the account is deactivated. + if user.DeactivatedAt != nil { + return ResetTokenStateUsed, ResetTokenFlowReset, nil + } + return ResetTokenStateValid, resetTokenFlowFor(user), nil } diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index 52ae211a3..1746a344e 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -781,13 +781,11 @@ func TestService_ResetTokenStatus(t *testing.T) { mockStore.AssertExpectations(t) }) - // Regression test for issue #89 / audit finding A03-006: a deactivated - // user (DeactivatedAt set) is also Active=false, but must report the - // "reset" flow, not "invite" -- the frontend must not show "Set your - // password" wording implying the token will activate the account, since - // ConfirmPasswordReset refuses to. Before the fix, resetTokenFlowFor did - // not exist and this branch keyed on !user.Active alone. - t.Run("valid token on deactivated user is valid + reset flow, not invite", func(t *testing.T) { + // Issue #421: ConfirmPasswordReset refuses deactivated accounts, so the + // status probe must not offer the form. It reports "used", the same state + // as an unknown or consumed token, so the endpoint does not reveal that the + // account is deactivated. + t.Run("unexpired token on deactivated user reports used + reset flow", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) @@ -802,7 +800,28 @@ func TestService_ResetTokenStatus(t *testing.T) { state, flow, err := service.ResetTokenStatus(ctx, "valid-token-deactivated") require.NoError(t, err) - assert.Equal(t, ResetTokenStateValid, state) + assert.Equal(t, ResetTokenStateUsed, state) + assert.Equal(t, ResetTokenFlowReset, flow) + + mockStore.AssertExpectations(t) + }) + + t.Run("expired token on deactivated user reports expired + reset flow", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + expiry := time.Now().Add(-time.Hour) + deactivatedAt := time.Now().Add(-2 * time.Hour) + mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")). + Return(&User{ + ID: "u5", Active: false, DeactivatedAt: &deactivatedAt, + PasswordResetToken: hashSessionToken("expired-deactivated"), PasswordResetExpiry: &expiry, + }, nil).Once() + + state, flow, err := service.ResetTokenStatus(ctx, "expired-deactivated") + require.NoError(t, err) + assert.Equal(t, ResetTokenStateExpired, state) assert.Equal(t, ResetTokenFlowReset, flow) mockStore.AssertExpectations(t) diff --git a/internal/server/reset_token_status_deactivated_test.go b/internal/server/reset_token_status_deactivated_test.go new file mode 100644 index 000000000..b90cd3136 --- /dev/null +++ b/internal/server/reset_token_status_deactivated_test.go @@ -0,0 +1,117 @@ +package server + +// Issue #421: the reset-token status endpoint must agree with the confirm +// endpoint for an admin-deactivated user. Both requests go through +// Handler.HandleRequest into the real auth.Service (only the store is mocked). + +import ( + "context" + "encoding/base64" + "encoding/json" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/jackc/pgx/v5" +) + +const deactivatedResetRawToken = "deactivated-user-reset-token" + +func newResetTokenHarness(t *testing.T) (*api.Handler, *auth.MockStore) { + t.Helper() + mockStore := new(auth.MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := auth.NewService(auth.ServiceConfig{ + Store: mockStore, + SessionDuration: time.Hour, + CSRFKey: auth.TestCSRFKey(), + }) + return api.NewHandler(api.HandlerConfig{AuthService: newAuthServiceAdapter(service)}), mockStore +} + +func deactivatedResetUser() *auth.User { + expiry := time.Now().Add(time.Hour) + deactivatedAt := time.Now().Add(-time.Hour) + return &auth.User{ + ID: "deactivated-user", + Email: "deactivated@example.com", + Active: false, + DeactivatedAt: &deactivatedAt, + PasswordResetToken: hashSessionTokenForTest(deactivatedResetRawToken), + PasswordResetExpiry: &expiry, + } +} + +func resetStatusRequest(token string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{"token": token}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "GET", + Path: "/api/auth/reset-password/status", + }, + }, + } +} + +func TestHandleRequest_ResetPasswordStatus_DeactivatedUserLooksLikeUnknownToken(t *testing.T) { + ctx := context.Background() + handler, mockStore := newResetTokenHarness(t) + + mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest(deactivatedResetRawToken)). + Return(deactivatedResetUser(), nil).Once() + mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest("unknown-token")). + Return((*auth.User)(nil), pgx.ErrNoRows).Once() + + deactivated, err := handler.HandleRequest(ctx, resetStatusRequest(deactivatedResetRawToken)) + require.NoError(t, err) + unknown, err := handler.HandleRequest(ctx, resetStatusRequest("unknown-token")) + require.NoError(t, err) + + require.Equal(t, 200, unknown.StatusCode, "body: %s", unknown.Body) + require.Equal(t, 200, deactivated.StatusCode, "body: %s", deactivated.Body) + var body map[string]string + require.NoError(t, json.Unmarshal([]byte(deactivated.Body), &body)) + assert.Equal(t, "used", body["state"]) + assert.Equal(t, "reset", body["flow"]) + assert.Equal(t, unknown.StatusCode, deactivated.StatusCode) + assert.Equal(t, unknown.Headers, deactivated.Headers, + "deactivated token must be indistinguishable from an unknown token") + assert.JSONEq(t, unknown.Body, deactivated.Body, + "deactivated token must be indistinguishable from an unknown token") +} + +func TestHandleRequest_ConfirmPasswordReset_DeactivatedUserRefused(t *testing.T) { + ctx := context.Background() + handler, mockStore := newResetTokenHarness(t) + + user := deactivatedResetUser() + mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest(deactivatedResetRawToken)). + Return(user, nil).Once() + mockStore.On("ConsumePasswordResetToken", mock.Anything, user.ID, user.PasswordResetToken). + Return(nil).Once() + + body, err := json.Marshal(map[string]string{ + "token": deactivatedResetRawToken, + "new_password": base64.StdEncoding.EncodeToString([]byte("Str0ng-Passw0rd-For-Test!")), + }) + require.NoError(t, err) + resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Content-Type": "application/json"}, + Body: string(body), + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "POST", + Path: "/api/auth/reset-password", + }, + }, + }) + require.NoError(t, err) + assert.Equal(t, 403, resp.StatusCode, "body: %s", resp.Body) +}