fix(api): keep group details out of user-write refusals for callers without view:groups

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)
+			}
+		})
+	}
+}

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

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)
+				}
+			})
+		}
+	}
+}