From 35568f6dec3a801d58c1a93fee40f1b9f81fb563 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 17:01:50 +0100 Subject: [PATCH 1/5] fix(api): keep group details out of the combination error for callers without view:groups A refused view-only plus write-capable group combination now names the groups and their split only when the caller holds view:groups. Other callers get "this group combination is not allowed". Applies to user create and update; the admin API key keeps the detailed message. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_users.go | 21 ++++++++- internal/api/handler_users_test.go | 68 ++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index f5548c983..1b705d79f 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -69,12 +69,29 @@ 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" + +// mapUserWriteError maps a user create/update failure like mapAuthError, except +// that a refused group combination only carries its group-by-group 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 { + if errors.Is(err, auth.ErrContradictoryGroups) { + if _, permErr := h.requirePermission(ctx, req, "view", "groups"); permErr != nil { + return NewClientError(400, groupCombinationDeniedMessage) + } + } + 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 +163,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..8bc582e48 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -367,3 +367,71 @@ 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) + } + }) + } + } +} From 9a3e6fb57de4083f0387b49fa967e26065253d99 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 17:12:49 +0100 Subject: [PATCH 2/5] test(api): pin the plain refusal when the view:groups check errors or the caller is a user API key Cover a permission check that returns an error (treated as not allowed to view groups) and a user API key judged by its own effective permissions. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_users_test.go | 63 ++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index 8bc582e48..1f930deea 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -435,3 +435,66 @@ func TestHandler_users_ContradictoryGroupsMessageDependsOnViewGroups(t *testing. } } } + +// 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()) + }) + } +} From 6fdff37b1a08d1810562564edcc1d5ff14664066 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 17:15:03 +0100 Subject: [PATCH 3/5] fix(api): keep group names and permissions out of grant refusals for callers without view:groups Extend mapUserWriteError so a user create/update refused by the permission ceiling returns a generic 403 unless the caller may view groups. The status codes are unchanged; callers with view:groups and the admin API key keep the detailed message. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_users.go | 25 ++++++++--- internal/api/handler_users_test.go | 68 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index 1b705d79f..8ba6ab6d0 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -79,15 +79,26 @@ func (h *Handler) createUser(ctx context.Context, req *events.LambdaFunctionURLR // 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 only carries its group-by-group explanation -// when the caller may view groups. A failed or erroring permission check gets -// the generic message. +// that a refused group combination or group grant 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 { - if errors.Is(err, auth.ErrContradictoryGroups) { - if _, permErr := h.requirePermission(ctx, req, "view", "groups"); permErr != nil { - return NewClientError(400, groupCombinationDeniedMessage) - } + 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): + 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) } diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index 1f930deea..e8e54b543 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -498,3 +498,71 @@ func TestHandler_users_ContradictoryGroupsMessageForUserAPIKey(t *testing.T) { }) } } + +// 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) + } + }) + } + } + } +} From c70d685514775519669d3012266858a51f516e4a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 17:34:54 +0100 Subject: [PATCH 4/5] fix(api): keep group data out of own-membership refusals for callers without view:groups Map a refused change to the caller's own group membership through the same view:groups check as the other group grant refusals, so a caller without it gets the generic 403 and callers with it keep the detail. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_users.go | 5 +- internal/api/handler_users_test.go | 58 +++++++++++++++++++ .../self_account_scope_dispatch_test.go | 12 +++- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index 8ba6ab6d0..5d836aaa2 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -84,7 +84,7 @@ const groupCombinationDeniedMessage = "this group combination is not allowed" 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 or group grant only carries its +// 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 { @@ -92,7 +92,8 @@ func (h *Handler) mapUserWriteError(ctx context.Context, req *events.LambdaFunct switch { case errors.Is(err, auth.ErrContradictoryGroups): status, generic = 400, groupCombinationDeniedMessage - case errors.Is(err, auth.ErrPermissionCeiling), errors.Is(err, auth.ErrPermissionNotGrantable): + case errors.Is(err, auth.ErrPermissionCeiling), errors.Is(err, auth.ErrPermissionNotGrantable), + errors.Is(err, auth.ErrSelfEscalation): status, generic = 403, groupGrantDeniedMessage default: return mapAuthError(err) diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index e8e54b543..796584f88 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -566,3 +566,61 @@ func TestHandler_users_GroupGrantRefusalMessageDependsOnViewGroups(t *testing.T) } } } + +// 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/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) From ffd2d3e4b83d5748c2d5429dfe740309aed9fbd1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 06:17:26 +0100 Subject: [PATCH 5/5] fix(auth): keep store errors out of grant-ceiling refusals When the grant ceiling could not resolve the acting user's permissions or account scope, the refusal wrapped the store error, so database connection text could reach the caller through the 403 message. Log the store error server-side and return only the fixed "could not resolve" refusal. The check still fails closed. Co-Authored-By: Claude Opus 5.5 --- .../api/handler_ceiling_store_error_test.go | 85 +++++++++++++++++++ internal/auth/group_ceiling.go | 10 ++- .../auth/group_ceiling_store_error_test.go | 48 +++++++++++ 3 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 internal/api/handler_ceiling_store_error_test.go create mode 100644 internal/auth/group_ceiling_store_error_test.go 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/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) +}