diff --git a/internal/api/apikey_usage_booking_test.go b/internal/api/apikey_usage_booking_test.go index 890096d7..3eb943e1 100644 --- a/internal/api/apikey_usage_booking_test.go +++ b/internal/api/apikey_usage_booking_test.go @@ -70,6 +70,8 @@ func TestHandleRequest_SessionAuthBooksNoAPIKeyUsage(t *testing.T) { Return(&Session{UserID: "session-user"}, nil).Once() mockAuth.On("GetUserPermissionsAPI", mock.Anything, "session-user"). Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "session-user"). + Return([]string{}, nil).Once() h := &Handler{auth: mockAuth} resp, err := h.HandleRequest(ctx, diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 14ad4859..a31ddc79 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -114,7 +114,10 @@ func (h *Handler) getCurrentUser(ctx context.Context, req *events.LambdaFunction // view-only key owned by an admin is not advertised as admin (issue #62); // - admin API key: {admin, *}. // -// IsAdmin is derived from that same set. +// IsAdmin is derived from that same set. UnrestrictedAccountAccess reports +// whether the caller passes the account-scope gate that global actions such +// as POST /api/recommendations/refresh enforce (issue #619), so the frontend +// can skip an automatic refresh the backend would refuse with 403. // // Auth: the route is AuthUser, which admits admin API key, user API // key, OR a bearer-token session (see router.go AuthLevel doc). The @@ -132,7 +135,15 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam return nil, err } if isUserAPIKey { - return h.userAPIKeyPermissions(ctx, keyID, keyOwnerID) + resp, keyErr := h.userAPIKeyPermissions(ctx, keyID, keyOwnerID) + if keyErr != nil { + return nil, keyErr + } + // Resolve the flag for the same session the refresh gate sees for + // this key: authorizeAPIKey puts the owner's ID and the key's ID on + // it, so the two stay in step if getAccountScope ever reads the key. + resp.UnrestrictedAccountAccess = h.unrestrictedAccountAccess(ctx, &Session{UserID: keyOwnerID, UserAPIKeyID: keyID}) + return resp, nil } userID, err := h.resolveAuthenticatedUserID(ctx, req) @@ -146,8 +157,9 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam // (matches the requireAdmin / requirePermission short-circuit). if userID == apiKeyAdminUserID { return &UserPermissionsResponse{ - Permissions: []PermissionEntry{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, - IsAdmin: true, + Permissions: []PermissionEntry{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + IsAdmin: true, + UnrestrictedAccountAccess: h.unrestrictedAccountAccess(ctx, &Session{UserID: userID}), }, nil } @@ -166,7 +178,28 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam logging.Errorf("getCurrentUserPermissions: GetUserPermissionsAPI returned %T, want []auth.APIPermission", raw) return nil, fmt.Errorf("GetUserPermissionsAPI returned unexpected payload type %T", raw) } - return permissionsResponse(apiPerms), nil + resp := permissionsResponse(apiPerms) + resp.UnrestrictedAccountAccess = h.unrestrictedAccountAccess(ctx, &Session{UserID: userID}) + return resp, nil +} + +// unrestrictedAccountAccess reports whether session passes +// requireUnrestrictedAccountScope, the gate on POST +// /api/recommendations/refresh, by running that same check rather than a +// copy of it. A scope lookup failure is logged and reported as false: the flag +// only steers the UI (false skips an automatic refresh), the refresh endpoint +// still enforces the scope itself, and failing the whole permissions response +// would drop the frontend to its coarser group-based gating. +func (h *Handler) unrestrictedAccountAccess(ctx context.Context, session *Session) bool { + err := h.requireUnrestrictedAccountScope(ctx, session) + if err == nil { + return true + } + if ce, isClient := IsClientError(err); isClient && ce.code == 403 { + return false // a restricted scope: the expected refusal + } + logging.Errorf("getCurrentUserPermissions: resolving account scope for user %s: %v", session.UserID, err) + return false } // userAPIKeyCaller reports whether the request authenticated with a user API diff --git a/internal/api/handler_auth_me_permissions_scope_test.go b/internal/api/handler_auth_me_permissions_scope_test.go new file mode 100644 index 00000000..4f96ba85 --- /dev/null +++ b/internal/api/handler_auth_me_permissions_scope_test.go @@ -0,0 +1,216 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + + "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/auth" +) + +// Issue #619: POST /api/recommendations/refresh refuses a caller whose account +// scope is restricted (#615), but the frontend fires that refresh +// automatically for stale data and had no way to tell a scoped caller apart, +// so scoped users saw a "refresh failed: permission denied" toast for an +// action they never asked for. GET /api/auth/me/permissions now reports +// unrestricted_account_access, resolved through the same gate the refresh +// enforces, so the frontend can skip the automatic refresh. + +const ( + meScopeKeyOwnerID = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" + meScopeKeyID = "key-619" + meScopeRawKey = "raw-user-key-619" +) + +// meScopeAuth returns a session caller holding view:recommendations whose +// allowed_accounts is accounts (nil means unrestricted). +func meScopeAuth(accounts []string) *MockAuthService { + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, scopedToken). + Return(&Session{UserID: scopedUserID}, nil).Maybe() + m.On("GetUserPermissionsAPI", mock.Anything, scopedUserID). + Return([]auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, nil).Maybe() + m.grantPermissionsScoped(viewRecommendationsOnly(), accounts) + return m +} + +func routeMePermissions(t *testing.T, h *Handler, req *events.LambdaFunctionURLRequest) *UserPermissionsResponse { + t.Helper() + res, err := NewRouter(h).Route(context.Background(), http.MethodGet, "/api/auth/me/permissions", req) + require.NoError(t, err) + out, ok := res.(*UserPermissionsResponse) + require.True(t, ok, "got %T", res) + return out +} + +func TestGetCurrentUserPermissions_ScopedSession_ReportsRestricted(t *testing.T) { + h := &Handler{auth: meScopeAuth([]string{scopedInAccount})} + + got := routeMePermissions(t, h, scopedRequest("")) + + assert.False(t, got.UnrestrictedAccountAccess, "a caller limited to a subset of accounts is not unrestricted") + assert.Equal(t, []PermissionEntry{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, got.Permissions, + "the permission set is reported unchanged") + assert.False(t, got.IsAdmin) +} + +func TestGetCurrentUserPermissions_UnrestrictedSession_ReportsUnrestricted(t *testing.T) { + h := &Handler{auth: meScopeAuth(nil)} + + got := routeMePermissions(t, h, scopedRequest("")) + + assert.True(t, got.UnrestrictedAccountAccess) +} + +// The wire key is always present, so the frontend can rely on it being a +// boolean rather than treating a missing field as either answer. +func TestGetCurrentUserPermissions_ScopedSession_WireField(t *testing.T) { + h := &Handler{auth: meScopeAuth([]string{scopedInAccount})} + req := scopedRequest("") + req.RequestContext.HTTP = events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: http.MethodGet, Path: "/api/auth/me/permissions", + } + + resp, err := h.HandleRequest(context.Background(), req) + + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", resp.Body) + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(resp.Body), &body), "body: %s", resp.Body) + assert.Equal(t, false, body["unrestricted_account_access"], "body: %s", resp.Body) +} + +// A user API key is gated by its owner's account scope on the refresh +// (authorizeAPIKey puts the owner on the session), so it is reported the same. +func TestGetCurrentUserPermissions_UserAPIKey_ReportsOwnerScope(t *testing.T) { + for name, tc := range map[string]struct { + accounts []string + want bool + }{ + "scoped owner": {accounts: []string{scopedInAccount}, want: false}, + "unrestricted owner": {accounts: []string{}, want: true}, + } { + t.Run(name, func(t *testing.T) { + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + m.On("ValidateUserAPIKeyAPI", mock.Anything, meScopeRawKey). + Return(&auth.UserAPIKey{ID: meScopeKeyID}, &auth.User{ID: meScopeKeyOwnerID}, nil).Once() + m.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, meScopeKeyID, meScopeKeyOwnerID). + Return([]auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, nil).Once() + m.On("GetAllowedAccountsAPI", mock.Anything, meScopeKeyOwnerID).Return(tc.accounts, nil).Once() + h := &Handler{auth: m} + + got := routeMePermissions(t, h, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": meScopeRawKey}, + }) + + assert.Equal(t, tc.want, got.UnrestrictedAccountAccess) + }) + } +} + +// A scope that cannot be established is reported as restricted (the +// conservative answer: the UI skips the automatic global refresh) without +// failing the permissions response the whole UI gates on. +func TestGetCurrentUserPermissions_ScopeLookupError_ReportsRestricted(t *testing.T) { + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + m.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Once() + m.On("GetUserPermissionsAPI", mock.Anything, scopedUserID). + Return([]auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, nil).Once() + m.On("GetAllowedAccountsAPI", mock.Anything, scopedUserID).Return(nil, errors.New("scope lookup failed")).Once() + h := &Handler{auth: m} + + got := routeMePermissions(t, h, scopedRequest("")) + + assert.False(t, got.UnrestrictedAccountAccess) + assert.Equal(t, []PermissionEntry{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, got.Permissions) +} + +func TestGetCurrentUserPermissions_AdminAPIKey_ReportsUnrestricted(t *testing.T) { + m := new(MockAuthService) + h := &Handler{auth: m, apiKey: "admin-key-619"} + + got := routeMePermissions(t, h, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "admin-key-619"}, + }) + + assert.True(t, got.UnrestrictedAccountAccess) + m.AssertNotCalled(t, "GetAllowedAccountsAPI", mock.Anything, mock.Anything) +} + +// The reported flag and the refresh gate must not drift: for the same caller, +// the flag is true exactly when POST /api/recommendations/refresh gets past the +// scope check, and false exactly when it is refused with the scope 403. +func TestGetCurrentUserPermissions_FlagMatchesRefreshGate(t *testing.T) { + t.Setenv("SCHEDULER_LAMBDA_ARN", "") + for name, accounts := range map[string][]string{ + "scoped": {scopedInAccount}, + "scoped to two accounts": {scopedInAccount, scopedOutAccount}, + "unrestricted (nil)": nil, + "unrestricted (empty)": {}, + "wildcard allowed list": {"*"}, + "wildcard plus an account": {"*", scopedInAccount}, + } { + t.Run(name, func(t *testing.T) { + f := newRefreshFixtureWithAuth(t, meScopeAuth(accounts)) + + got := routeMePermissions(t, f.handler, scopedRequest("")) + res, refreshErr := f.refresh(scopedRequest("")) + + if got.UnrestrictedAccountAccess { + require.NoError(t, refreshErr, "flag says unrestricted, so the refresh must pass the gate") + assert.IsType(t, &RefreshResponse{}, res) + return + } + requireForbiddenUnrestricted(t, res, refreshErr) + f.assertNothingStarted(t) + }) + } +} + +// The same parity for a user API key: /me/permissions and the refresh both see +// the key's session (owner ID plus key ID), so the flag agrees with what the +// refresh does when the same key calls it. +func TestGetCurrentUserPermissions_UserAPIKey_FlagMatchesRefreshGate(t *testing.T) { + t.Setenv("SCHEDULER_LAMBDA_ARN", "") + for name, accounts := range map[string][]string{ + "scoped owner": {scopedInAccount}, + "unrestricted owner": nil, + "wildcard owner": {"*"}, + } { + t.Run(name, func(t *testing.T) { + m := new(MockAuthService) + m.On("ValidateUserAPIKeyAPI", mock.Anything, meScopeRawKey). + Return(&auth.UserAPIKey{ID: meScopeKeyID}, &auth.User{ID: meScopeKeyOwnerID}, nil).Maybe() + m.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, meScopeKeyID, meScopeKeyOwnerID). + Return([]auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, nil).Maybe() + m.On("HasAPIKeyPermissionAPI", mock.Anything, meScopeRawKey, auth.ActionView, auth.ResourceRecommendations). + Return(meScopeKeyOwnerID, meScopeKeyID, true, nil).Maybe() + m.On("GetAllowedAccountsAPI", mock.Anything, meScopeKeyOwnerID).Return(accounts, nil).Maybe() + f := newRefreshFixtureWithAuth(t, m) + keyRequest := func() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": meScopeRawKey}} + } + + got := routeMePermissions(t, f.handler, keyRequest()) + res, refreshErr := f.refresh(keyRequest()) + + m.AssertCalled(t, "HasAPIKeyPermissionAPI", mock.Anything, meScopeRawKey, auth.ActionView, auth.ResourceRecommendations) + if got.UnrestrictedAccountAccess { + require.NoError(t, refreshErr, "flag says unrestricted, so the refresh must pass the gate") + assert.IsType(t, &RefreshResponse{}, res) + return + } + requireForbiddenUnrestricted(t, res, refreshErr) + f.assertNothingStarted(t) + }) + } +} diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 454c21fb..5cc0073f 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1430,12 +1430,14 @@ func TestHandler_getCurrentUserPermissions_RegularUser(t *testing.T) { {Action: "view", Resource: "plans"}, } mockAuth.On("GetUserPermissionsAPI", ctx, "user-1").Return(perms, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "user-1").Return([]string{}, nil).Once() handler := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}} result, err := handler.getCurrentUserPermissions(ctx, req) require.NoError(t, err) assert.False(t, result.IsAdmin) + assert.True(t, result.UnrestrictedAccountAccess, "no allowed_accounts restriction means unrestricted") require.Len(t, result.Permissions, 2) assert.Equal(t, PermissionEntry{Action: "view", Resource: "recommendations"}, result.Permissions[0]) assert.Equal(t, PermissionEntry{Action: "view", Resource: "plans"}, result.Permissions[1]) @@ -1455,6 +1457,7 @@ func TestHandler_getCurrentUserPermissions_Admin(t *testing.T) { {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, } mockAuth.On("GetUserPermissionsAPI", ctx, "admin-1").Return(perms, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "admin-1").Return([]string{}, nil).Once() handler := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-tok"}} @@ -1525,6 +1528,7 @@ func TestHandler_getCurrentUserPermissions_AdminAPIKey(t *testing.T) { // short-circuits to {admin, *} + is_admin=true rather than calling // GetUserPermissionsAPI (which would fail to find an admin-api-key row). assert.True(t, result.IsAdmin) + assert.True(t, result.UnrestrictedAccountAccess, "the admin API key is positively unrestricted") require.Len(t, result.Permissions, 1) assert.Equal(t, PermissionEntry{Action: auth.ActionAdmin, Resource: auth.ResourceAll}, result.Permissions[0]) } @@ -1546,6 +1550,7 @@ func TestHandler_getCurrentUserPermissions_UserAPIKey(t *testing.T) { {Action: "view", Resource: "recommendations"}, } mockAuth.On("GetAPIKeyEffectivePermissionsAPI", ctx, "key-42", "user-42").Return(perms, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "user-42").Return([]string{}, nil).Once() handler := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": "user-key"}} diff --git a/internal/api/router_authuser_test.go b/internal/api/router_authuser_test.go index 1fedbb2e..505620e0 100644 --- a/internal/api/router_authuser_test.go +++ b/internal/api/router_authuser_test.go @@ -91,6 +91,7 @@ func TestRouterAuthUser_SessionPrincipalAvoidsSecondValidation(t *testing.T) { session := &Session{UserID: "session-user"} mockAuth.On("ValidateSession", ctx, "session-token").Return(session, nil).Once() mockAuth.On("GetUserPermissionsAPI", mock.Anything, "session-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "session-user").Return([]string{}, nil).Once() h := &Handler{auth: mockAuth} r := NewRouter(h) req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer session-token"}} @@ -108,6 +109,7 @@ func TestRouterAuthUser_UserAPIKeyPrincipalAvoidsSecondValidation(t *testing.T) user := &auth.User{ID: "api-key-user", Email: "user@example.com"} mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return(&auth.UserAPIKey{ID: "key-1"}, user, nil).Once() mockAuth.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, "key-1", "api-key-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "api-key-user").Return([]string{}, nil).Once() h := &Handler{auth: mockAuth} r := NewRouter(h) req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": "user-key"}} @@ -142,6 +144,7 @@ func TestHandleRequest_SessionPrincipalValidatedOnce(t *testing.T) { session := &Session{UserID: "session-user"} mockAuth.On("ValidateSession", ctx, "session-token").Return(session, nil).Once() mockAuth.On("GetUserPermissionsAPI", mock.Anything, "session-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "session-user").Return([]string{}, nil).Once() h := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"Authorization": "Bearer session-token"}, @@ -164,6 +167,7 @@ func TestHandleRequest_UserAPIKeyPrincipalValidatedOnce(t *testing.T) { user := &auth.User{ID: "api-key-user", Email: "user@example.com"} mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return(&auth.UserAPIKey{ID: "key-1"}, user, nil).Once() mockAuth.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, "key-1", "api-key-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "api-key-user").Return([]string{}, nil).Once() h := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"X-API-Key": "user-key"}, diff --git a/internal/api/types.go b/internal/api/types.go index 752450a5..6d3d38ae 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -555,9 +555,14 @@ type PermissionEntry struct { // the user's groups; for a user API key, the key's permissions intersected // with its owner's (issue #62); for the admin API key, {admin, *}. // IsAdmin mirrors whether that effective set contains the {admin, *} wildcard. +// UnrestrictedAccountAccess reports whether the caller's account scope covers +// every account, which global actions such as POST +// /api/recommendations/refresh require (issue #619). Like the permission +// entries it is for UX gating only; the backend still enforces the scope. type UserPermissionsResponse struct { - Permissions []PermissionEntry `json:"permissions"` - IsAdmin bool `json:"is_admin"` + Permissions []PermissionEntry `json:"permissions"` + IsAdmin bool `json:"is_admin"` + UnrestrictedAccountAccess bool `json:"unrestricted_account_access"` } // MFA enrollment + lifecycle DTOs (issue #497). Passwords carried by