diff --git a/internal/api/handler_ceiling_store_error_test.go b/internal/api/handler_ceiling_store_error_test.go new file mode 100644 index 000000000..cb8826a02 --- /dev/null +++ b/internal/api/handler_ceiling_store_error_test.go @@ -0,0 +1,85 @@ +package api + +import ( + "context" + "encoding/base64" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A store failure while resolving the acting user's permissions must reach the +// caller as the plain 403, even when the caller may view groups. + +const ceilingStoreFailure = "failed to connect to `user=app_user database=app`: 10.1.2.3:5432 (10.1.2.3): dial error" + +// failingActorStore fails the acting user's lookup; the target-email lookup +// reports "no such user" so a create reaches the grant check. +type failingActorStore struct { + auth.StoreInterface +} + +func (failingActorStore) GetUserByEmail(context.Context, string) (*auth.User, error) { + return nil, pgx.ErrNoRows +} + +func (failingActorStore) GetUserByID(context.Context, string) (*auth.User, error) { + return nil, errors.New(ceilingStoreFailure) +} + +// realWriteAuth routes the user and group create entry points to a real +// auth.Service and keeps everything else on the mock. +type realWriteAuth struct { + *MockAuthService + svc *auth.Service +} + +func (a realWriteAuth) CreateUserAPI(ctx context.Context, actor string, req auth.APICreateUserRequest) (*auth.APICreateUserResponse, error) { + return a.svc.CreateUserAPI(ctx, actor, req) +} + +func (a realWriteAuth) CreateGroupAPI(ctx context.Context, actor string, req auth.APICreateGroupRequest) (*auth.APIGroup, error) { + return a.svc.CreateGroupAPI(ctx, actor, req) +} + +func TestHandler_writes_CeilingStoreErrorOmitsDetailForGroupViewer(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "ops@example.com"} + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + mockAuth.grantAdmin() + svc := auth.NewService(auth.ServiceConfig{Store: failingActorStore{}}) + h := &Handler{auth: realWriteAuth{MockAuthService: mockAuth, svc: svc}} + headers := map[string]string{"Authorization": "Bearer tok"} + password := base64.StdEncoding.EncodeToString([]byte("Zk9#vQ2mXr!41wLp")) + + cases := map[string]func() error{ + "create user": func() error { + _, err := h.createUser(ctx, &events.LambdaFunctionURLRequest{Headers: headers, + Body: `{"email": "new@example.com", "password": "` + password + `", "groups": ["00000000-0000-5000-8000-000000000005"]}`}) + return err + }, + "create group": func() error { + _, err := h.createGroup(ctx, &events.LambdaFunctionURLRequest{Headers: headers, + Body: `{"name": "Readers", "permissions": [{"action": "view", "resource": "recommendations"}], "allowed_accounts": ["*"]}`}) + return err + }, + } + for name, call := range cases { + t.Run(name, func(t *testing.T) { + err := call() + + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 403, ce.code) + for _, needle := range []string{"10.1.2.3", "app_user", "dial"} { + assert.NotContains(t, err.Error(), needle) + } + }) + } +} diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index f5548c983..5d836aaa2 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -69,12 +69,41 @@ func (h *Handler) createUser(ctx context.Context, req *events.LambdaFunctionURLR // requested groups at the actor's own grant ceiling (issue #226). user, err := h.auth.CreateUserAPI(ctx, session.UserID, createReq) if err != nil { - return nil, mapAuthError(err) + return nil, h.mapUserWriteError(ctx, req, err) } return user, nil } +// groupCombinationDeniedMessage is what a caller who cannot view groups is +// told about a refused group combination: no group name and no view/write split. +const groupCombinationDeniedMessage = "this group combination is not allowed" + +// groupGrantDeniedMessage is what a caller who cannot view groups is told about +// a refused group grant: no group name and no permission. +const groupGrantDeniedMessage = "you cannot grant one or more of the requested groups" + +// mapUserWriteError maps a user create/update failure like mapAuthError, except +// that a refused group combination, group grant or self-escalation only carries its +// group-specific explanation when the caller may view groups. A failed or +// erroring permission check gets the generic message. +func (h *Handler) mapUserWriteError(ctx context.Context, req *events.LambdaFunctionURLRequest, err error) error { + status, generic := 0, "" + switch { + case errors.Is(err, auth.ErrContradictoryGroups): + status, generic = 400, groupCombinationDeniedMessage + case errors.Is(err, auth.ErrPermissionCeiling), errors.Is(err, auth.ErrPermissionNotGrantable), + errors.Is(err, auth.ErrSelfEscalation): + status, generic = 403, groupGrantDeniedMessage + default: + return mapAuthError(err) + } + if _, permErr := h.requirePermission(ctx, req, "view", "groups"); permErr != nil { + return NewClientError(status, generic) + } + return mapAuthError(err) +} + // mapAuthError maps internal/auth sentinel errors to the appropriate // ClientError status code so validation failures surface to the user // as a 4xx with the real message instead of a generic 500. Unrecognized @@ -146,7 +175,7 @@ func (h *Handler) updateUser(ctx context.Context, req *events.LambdaFunctionURLR // the self-escalation guard (issue #907). user, err := h.auth.UpdateUserAPI(ctx, session.UserID, userID, updateReq) if err != nil { - return nil, mapAuthError(err) + return nil, h.mapUserWriteError(ctx, req, err) } return user, nil diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index 8b45be530..796584f88 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -367,3 +367,260 @@ func TestHandler_users_ContradictoryGroupsMapTo400(t *testing.T) { }) } } + +// A refused group combination names the groups and their view/write split only +// for a caller who may view groups; any other caller gets the plain refusal. +func TestHandler_users_ContradictoryGroupsMessageDependsOnViewGroups(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "ops@example.com"} + refusal := fmt.Errorf(`%w: "Viewers" is view-only but "Plan Authors" grants write permissions; assign a view-only group or write-capable groups, not both`, + auth.ErrContradictoryGroups) + groups := `["00000000-0000-5000-8000-000000000004", "00000000-0000-5000-8000-000000000003"]` + const userID = "11111111-1111-1111-1111-111111111111" + writeOnly := []auth.Permission{ + {Action: auth.ActionCreate, Resource: auth.ResourceUsers}, + {Action: auth.ActionUpdate, Resource: auth.ResourceUsers}, + } + withView := append([]auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceGroups}}, writeOnly...) + + type caller struct { + perms []auth.Permission + apiKey bool + detail bool + headers map[string]string + } + callers := map[string]caller{ + "without view groups": {perms: writeOnly, headers: map[string]string{"Authorization": "Bearer tok"}}, + "with view groups": {perms: withView, detail: true, headers: map[string]string{"Authorization": "Bearer tok"}}, + "admin api key": {apiKey: true, detail: true, headers: map[string]string{"X-API-Key": "admin-secret"}}, + } + ops := map[string]func(h *Handler, m *MockAuthService, headers map[string]string) (any, error){ + "create": func(h *Handler, m *MockAuthService, headers map[string]string) (any, error) { + m.On("CreateUserAPI", ctx, mock.Anything, mock.Anything).Return(nil, refusal).Once() + password := base64.StdEncoding.EncodeToString([]byte("password123")) + return h.createUser(ctx, &events.LambdaFunctionURLRequest{Headers: headers, + Body: `{"email": "new@example.com", "password": "` + password + `", "groups": ` + groups + `}`}) + }, + "update": func(h *Handler, m *MockAuthService, headers map[string]string) (any, error) { + m.On("UpdateUserAPI", ctx, mock.Anything, userID, mock.Anything).Return(nil, refusal).Once() + return h.updateUser(ctx, &events.LambdaFunctionURLRequest{Headers: headers, Body: `{"groups": ` + groups + `}`}, userID) + }, + } + for opName, op := range ops { + for callerName, c := range callers { + t.Run(opName+"/"+callerName, func(t *testing.T) { + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + if c.apiKey { + h.apiKey = "admin-secret" + } else { + mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + mockAuth.grantPermissions(c.perms) + } + + _, err := op(h, mockAuth, c.headers) + + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + if c.detail { + assert.Contains(t, err.Error(), `"Viewers" is view-only`) + return + } + assert.Equal(t, groupCombinationDeniedMessage, err.Error()) + for _, leaked := range []string{"Viewers", "Plan Authors", "view-only", "write", "00000000-0000-5000"} { + assert.NotContains(t, err.Error(), leaked) + } + }) + } + } +} + +// A permission check that itself errors must not be read as "may view groups": +// the caller gets the plain refusal. +func TestHandler_users_ContradictoryGroupsGenericWhenViewGroupsCheckErrors(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "ops@example.com"} + refusal := fmt.Errorf(`%w: "Viewers" is view-only but "Plan Authors" grants write permissions`, auth.ErrContradictoryGroups) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + mockAuth.On("HasPermissionAPI", ctx, session.UserID, "view", "groups").Return(false, fmt.Errorf("store unavailable")).Once() + mockAuth.On("HasPermissionAPI", ctx, session.UserID, "create", "users").Return(true, nil) + mockAuth.On("HasPermissionAPI", ctx, session.UserID, "update", "users").Return(true, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe() + mockAuth.On("UpdateUserAPI", ctx, mock.Anything, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(nil, refusal).Once() + h := &Handler{auth: mockAuth} + + _, err := h.updateUser(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer tok"}, + Body: `{"groups": ["00000000-0000-5000-8000-000000000004"]}`, + }, "11111111-1111-1111-1111-111111111111") + + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Equal(t, groupCombinationDeniedMessage, err.Error()) +} + +// A user API key is judged by its own effective permissions: a key without +// view:groups gets the plain refusal even if its owner could view groups. +func TestHandler_users_ContradictoryGroupsMessageForUserAPIKey(t *testing.T) { + ctx := context.Background() + refusal := fmt.Errorf(`%w: "Viewers" is view-only but "Plan Authors" grants write permissions`, auth.ErrContradictoryGroups) + const userID = "11111111-1111-1111-1111-111111111111" + + for name, canViewGroups := range map[string]bool{"key without view groups": false, "key with view groups": true} { + t.Run(name, func(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "user-key", "view", "groups"). + Return("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", "key-1", canViewGroups, nil) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "user-key", "update", "users"). + Return("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", "key-1", true, nil) + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(true, nil).Maybe() + mockAuth.On("UpdateUserAPI", ctx, mock.Anything, userID, mock.Anything).Return(nil, refusal).Once() + h := &Handler{auth: mockAuth} + + _, err := h.updateUser(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "user-key"}, + Body: `{"groups": ["00000000-0000-5000-8000-000000000004"]}`, + }, userID) + + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + if canViewGroups { + assert.Contains(t, err.Error(), `"Viewers" is view-only`) + return + } + assert.Equal(t, groupCombinationDeniedMessage, err.Error()) + }) + } +} + +// A refused group grant keeps the 403 status; it names the group and the +// permission only for a caller who may view groups. +func TestHandler_users_GroupGrantRefusalMessageDependsOnViewGroups(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "ops@example.com"} + groups := `["00000000-0000-5000-8000-000000000004"]` + const userID = "11111111-1111-1111-1111-111111111111" + refusals := map[string]error{ + "ceiling": fmt.Errorf(`%w: granting membership in "Plan Authors" would grant create:plans, beyond your own permissions`, auth.ErrPermissionCeiling), + "reserved": fmt.Errorf(`%w: granting membership in "Plan Authors" would grant create:plans, which is reserved`, + auth.ErrPermissionNotGrantable), + } + writeOnly := []auth.Permission{ + {Action: auth.ActionCreate, Resource: auth.ResourceUsers}, + {Action: auth.ActionUpdate, Resource: auth.ResourceUsers}, + } + withView := append([]auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceGroups}}, writeOnly...) + callers := map[string]struct { + perms []auth.Permission + apiKey bool + detail bool + headers map[string]string + }{ + "without view groups": {perms: writeOnly, headers: map[string]string{"Authorization": "Bearer tok"}}, + "with view groups": {perms: withView, detail: true, headers: map[string]string{"Authorization": "Bearer tok"}}, + "admin api key": {apiKey: true, detail: true, headers: map[string]string{"X-API-Key": "admin-secret"}}, + } + for refName, refusal := range refusals { + for _, op := range []string{"create", "update"} { + for callerName, c := range callers { + t.Run(refName+"/"+op+"/"+callerName, func(t *testing.T) { + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + if c.apiKey { + h.apiKey = "admin-secret" + } else { + mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + mockAuth.grantPermissions(c.perms) + } + var err error + if op == "create" { + mockAuth.On("CreateUserAPI", ctx, mock.Anything, mock.Anything).Return(nil, refusal).Once() + password := base64.StdEncoding.EncodeToString([]byte("password123")) + _, err = h.createUser(ctx, &events.LambdaFunctionURLRequest{Headers: c.headers, + Body: `{"email": "new@example.com", "password": "` + password + `", "groups": ` + groups + `}`}) + } else { + mockAuth.On("UpdateUserAPI", ctx, mock.Anything, userID, mock.Anything).Return(nil, refusal).Once() + _, err = h.updateUser(ctx, &events.LambdaFunctionURLRequest{Headers: c.headers, Body: `{"groups": ` + groups + `}`}, userID) + } + + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + if c.detail { + assert.Contains(t, err.Error(), `"Plan Authors"`) + assert.Contains(t, err.Error(), "create:plans") + return + } + assert.Equal(t, groupGrantDeniedMessage, err.Error()) + for _, leaked := range []string{"Plan Authors", "create:plans", "plans", "reserved", "beyond"} { + assert.NotContains(t, err.Error(), leaked) + } + }) + } + } + } +} + +// A refused change to the caller's own group membership keeps the 403 status; +// it names the group, permission or account only for a caller who may view groups. +func TestHandler_users_SelfEditRefusalMessageDependsOnViewGroups(t *testing.T) { + ctx := context.Background() + const selfID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + session := &Session{UserID: selfID, Email: "ops@example.com"} + groups := `["00000000-0000-5000-8000-000000000004"]` + refusals := map[string]struct { + err error + detail string + }{ + "reserved verb": { + err: fmt.Errorf(`%w: joining "Plan Authors" would grant you create:plans, which is reserved; ask another administrator to add you`, + auth.ErrSelfEscalation), + detail: `"Plan Authors"`, + }, + "account gap": { + err: fmt.Errorf(`%w: this change would give you access to cloud account "acct-4242", beyond your current account scope`, + auth.ErrSelfEscalation), + detail: `"acct-4242"`, + }, + } + writeOnly := []auth.Permission{{Action: auth.ActionUpdate, Resource: auth.ResourceUsers}} + withView := append([]auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceGroups}}, writeOnly...) + + for refName, r := range refusals { + for callerName, c := range map[string]struct { + perms []auth.Permission + detail bool + }{"without view groups": {writeOnly, false}, "with view groups": {withView, true}} { + t.Run(refName+"/"+callerName, func(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + mockAuth.grantPermissions(c.perms) + mockAuth.On("UpdateUserAPI", ctx, mock.Anything, selfID, mock.Anything).Return(nil, r.err).Once() + h := &Handler{auth: mockAuth} + + _, err := h.updateUser(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer tok"}, + Body: `{"groups": ` + groups + `}`, + }, selfID) + + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + if c.detail { + assert.Contains(t, err.Error(), r.detail) + return + } + assert.Equal(t, groupGrantDeniedMessage, err.Error()) + for _, leaked := range []string{"Plan Authors", "create:plans", "acct-4242", "account", "reserved"} { + assert.NotContains(t, err.Error(), leaked) + } + }) + } + } +} diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index d9d588d8f..750220a3d 100644 --- a/internal/auth/group_ceiling.go +++ b/internal/auth/group_ceiling.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" ) // Grant ceiling for group-permission writes (issues #1550, #1629). @@ -52,7 +54,9 @@ func (s *Service) grantCeilingPermissions(ctx context.Context, actorUserID strin } perms, err := s.GetUserPermissions(ctx, actorUserID) if err != nil { - return nil, fmt.Errorf("%w: could not resolve the acting user's permissions: %w", ErrPermissionCeiling, err) + // The store error stays in the server log; the refusal text reaches callers. + logging.Errorf("group ceiling: resolving permissions for actor %s: %v", actorUserID, err) + return nil, fmt.Errorf("%w: could not resolve the acting user's permissions", ErrPermissionCeiling) } return perms, nil } @@ -278,7 +282,9 @@ func (s *Service) grantCeilingAccounts(ctx context.Context, actorUserID string) } authCtx, err := s.BuildAuthContext(ctx, actorUserID) if err != nil { - return nil, fmt.Errorf("%w: could not resolve the acting user's account scope: %w", ErrPermissionCeiling, err) + // The store error stays in the server log; the refusal text reaches callers. + logging.Errorf("group ceiling: resolving account scope for actor %s: %v", actorUserID, err) + return nil, fmt.Errorf("%w: could not resolve the acting user's account scope", ErrPermissionCeiling) } // A PARTIAL resolution can WIDEN the actor's scope, which is the case that // matters and the one an earlier version of this guard missed. diff --git a/internal/auth/group_ceiling_store_error_test.go b/internal/auth/group_ceiling_store_error_test.go new file mode 100644 index 000000000..9160de6ee --- /dev/null +++ b/internal/auth/group_ceiling_store_error_test.go @@ -0,0 +1,48 @@ +package auth + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Store failures while resolving the acting user must keep the fail-closed +// refusal but not carry the store's connection detail in its text. + +const ceilingStoreFailure = "failed to connect to `user=app_user database=app`: 10.1.2.3:5432 (10.1.2.3): dial error" + +var ceilingStoreFailureNeedles = []string{"10.1.2.3", "app_user", "dial"} + +func assertCeilingRefusalWithoutStoreDetail(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + for _, needle := range ceilingStoreFailureNeedles { + assert.NotContains(t, err.Error(), needle) + } +} + +func TestGrantCeilingPermissions_StoreErrorOmitsDetail(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + mockStore.On("GetUserByID", ctx, ceilingActorID).Return(nil, errors.New(ceilingStoreFailure)) + s := newCeilingService(t, mockStore) + + _, err := s.grantCeilingPermissions(ctx, ceilingActorID) + + assertCeilingRefusalWithoutStoreDetail(t, err) +} + +func TestGrantCeilingAccounts_StoreErrorOmitsDetail(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + mockStore.On("GetUserByID", ctx, ceilingActorID).Return(nil, errors.New(ceilingStoreFailure)) + s := newCeilingService(t, mockStore) + + _, err := s.grantCeilingAccounts(ctx, ceilingActorID) + + assertCeilingRefusalWithoutStoreDetail(t, err) +} diff --git a/internal/server/self_account_scope_dispatch_test.go b/internal/server/self_account_scope_dispatch_test.go index cd9797975..786e5b5b0 100644 --- a/internal/server/self_account_scope_dispatch_test.go +++ b/internal/server/self_account_scope_dispatch_test.go @@ -125,9 +125,15 @@ func TestSelfAccountScopeDispatch_JoinWiderGroupRefused(t *testing.T) { // check untouched. func TestSelfAccountScopeDispatch_LeaveScopingGroupRefused(t *testing.T) { viewers := &auth.Group{ - ID: "55555555-5555-4555-8555-555555555556", - Name: "Viewers", - Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + ID: "55555555-5555-4555-8555-555555555556", + Name: "Viewers", + // view:groups keeps the refusal detail visible: the mock store hands + // back one shared user whose groups the refused edit has already + // rewritten, so the caller is judged by this group alone. + Permissions: []auth.Permission{ + {Action: auth.ActionView, Resource: auth.ResourceRecommendations}, + {Action: auth.ActionView, Resource: auth.ResourceGroups}, + }, } handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID, viewers.ID}) mockStore.On("GetGroup", mock.Anything, viewers.ID).Return(viewers, nil)