Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions internal/api/apikey_usage_booking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 38 additions & 5 deletions internal/api/handler_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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
}

Expand All @@ -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
Expand Down
216 changes: 216 additions & 0 deletions internal/api/handler_auth_me_permissions_scope_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
5 changes: 5 additions & 0 deletions internal/api/handler_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand All @@ -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"}}
Expand Down Expand Up @@ -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])
}
Expand All @@ -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"}}
Expand Down
4 changes: 4 additions & 0 deletions internal/api/router_authuser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}}
Expand All @@ -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"}}
Expand Down Expand Up @@ -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"},
Expand All @@ -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"},
Expand Down
9 changes: 7 additions & 2 deletions internal/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading