sec(auth): enforce a grant ceiling and system-managed guard on group writes

internal/api/handler.go

@@ -222,7 +222,10 @@ func NewHandler(cfg HandlerConfig) *Handler {
 // that would otherwise resolve group-derived permissions for it must treat it
 // as full-access up front (the API key is an infrastructure credential, not a
 // user). See requirePermission / requireAdmin / getAllowedAccounts.
-const apiKeyAdminUserID = "admin-api-key"
+// It is also the actor identity threaded into the group-write grant ceiling,
+// which recognizes it and measures the key against a bare {admin, *} holding
+// (auth.AdminAPIKeyActorID) rather than failing the user lookup.
+const apiKeyAdminUserID = auth.AdminAPIKeyActorID
 
 // requirePermission validates authentication and checks if the user holds the
 // specified permission. The stateless admin API key bypasses the per-user

internal/api/handler_coverage_test.go

@@ -502,7 +502,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil)
 		mockAuth.grantAdmin()
-		mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(map[string]string{"id": "new-group"}, nil)
+		mockAuth.On("CreateGroupAPI", ctx, "admin", mock.Anything).Return(map[string]string{"id": "new-group"}, nil)
 
 		h := &Handler{auth: mockAuth}
 		router := NewRouter(h)
@@ -540,7 +540,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil)
 		mockAuth.grantAdmin()
-		mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(map[string]string{}, nil)
+		mockAuth.On("UpdateGroupAPI", ctx, "admin", "11111111-1111-1111-1111-111111111111", mock.Anything).Return(map[string]string{}, nil)
 
 		h := &Handler{auth: mockAuth}
 		router := NewRouter(h)

internal/api/handler_groups.go

@@ -4,6 +4,7 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"errors"
 
 	"github.com/LeanerCloud/CUDly/internal/auth"
 	"github.com/LeanerCloud/CUDly/pkg/logging"
@@ -49,14 +50,32 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL
 		return nil, NewClientError(400, "invalid request body")
 	}
 
-	group, err := h.auth.CreateGroupAPI(ctx, createReq)
+	group, err := h.auth.CreateGroupAPI(ctx, session.UserID, createReq)
 	if err != nil {
-		return nil, err
+		return nil, mapGroupAuthError(err)
 	}
 
 	return group, nil
 }
 
+// mapGroupAuthError maps the group-write sentinels to 403 ClientErrors so a
+// refused grant surfaces the specific permission that was refused instead of
+// a generic 500. Kept separate from mapAuthError (handler_users.go) because
+// that switch is already at the cyclomatic limit and the two sentinel sets
+// are disjoint. Unrecognized errors pass through for handleRequestError.
+func mapGroupAuthError(err error) error {
+	switch {
+	case errors.Is(err, auth.ErrInvalidPermission):
+		// Malformed input, not an authorization failure.
+		return NewClientError(400, err.Error())
+	case errors.Is(err, auth.ErrPermissionCeiling),
+		errors.Is(err, auth.ErrPermissionNotGrantable),
+		errors.Is(err, auth.ErrSystemManagedGroup):
+		return NewClientError(403, err.Error())
+	}
+	return err
+}
+
 // getGroup handles GET /api/groups/{id}.
 func (h *Handler) getGroup(ctx context.Context, req *events.LambdaFunctionURLRequest, groupID string) (any, error) {
 	// Validate UUID format to prevent injection attacks
@@ -83,18 +102,19 @@ func (h *Handler) updateGroup(ctx context.Context, req *events.LambdaFunctionURL
 		return nil, err
 	}
 
-	if _, err := h.requirePermission(ctx, req, "update", "groups"); err != nil {
+	session, err := h.requirePermission(ctx, req, "update", "groups")
+	if err != nil {
 		return nil, err
 	}
 
 	var updateReq auth.APIUpdateGroupRequest
-	if err := json.Unmarshal([]byte(req.Body), &updateReq); err != nil {
+	if unmarshalErr := json.Unmarshal([]byte(req.Body), &updateReq); unmarshalErr != nil {
 		return nil, NewClientError(400, "invalid request body")
 	}
 
-	group, err := h.auth.UpdateGroupAPI(ctx, groupID, updateReq)
+	group, err := h.auth.UpdateGroupAPI(ctx, session.UserID, groupID, updateReq)
 	if err != nil {
-		return nil, err
+		return nil, mapGroupAuthError(err)
 	}
 
 	return group, nil
@@ -112,7 +132,7 @@ func (h *Handler) deleteGroup(ctx context.Context, req *events.LambdaFunctionURL
 	}
 
 	if err := h.auth.DeleteGroup(ctx, groupID); err != nil {
-		return nil, err
+		return nil, mapGroupAuthError(err)
 	}
 
 	return map[string]string{"status": "group deleted"}, nil

internal/api/handler_groups_ceiling_test.go

@@ -0,0 +1,140 @@
+package api
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/LeanerCloud/CUDly/internal/auth"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Handler-level coverage for the group grant ceiling (issues #1550, #1629).
+//
+// The ceiling itself is enforced in internal/auth; what these tests pin is
+// the wiring the auth package cannot see: that the AUTHENTICATED session's
+// user ID reaches the service as the actor (a ceiling measured against the
+// wrong principal is no ceiling), and that a refusal surfaces as a 403
+// naming the permission rather than a generic 500.
+
+const ceilingGroupID = "11111111-1111-1111-1111-111111111111"
+
+func TestUpdateGroup_ThreadsSessionActorAndMaps403(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil)
+	mockAuth.grantAdmin()
+
+	refusal := fmt.Errorf("%w: execute:purchases is reserved for separation of duties (issue #923) and cannot be granted through the API",
+		auth.ErrPermissionNotGrantable)
+	// The actor argument is asserted EXACTLY: if updateGroup passed anything
+	// other than the validated session's user ID, this expectation would not
+	// match and the mock would fail the test.
+	mockAuth.On("UpdateGroupAPI", ctx, session.UserID, ceilingGroupID, mock.Anything).
+		Return(nil, refusal).Once()
+
+	h := &Handler{auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    `{"permissions":[{"action":"execute","resource":"purchases"}]}`,
+	}
+
+	result, err := h.updateGroup(ctx, req, ceilingGroupID)
+
+	require.Error(t, err)
+	assert.Nil(t, result)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "a refused grant must map to a client error, not a 500")
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, err.Error(), "execute:purchases")
+}
+
+func TestCreateGroup_ThreadsSessionActorAndMaps403(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil)
+	mockAuth.grantAdmin()
+
+	refusal := fmt.Errorf("%w: cannot grant delete:accounts because your own permissions do not include it (or not at the requested scope)",
+		auth.ErrPermissionCeiling)
+	mockAuth.On("CreateGroupAPI", ctx, session.UserID, mock.Anything).
+		Return(nil, refusal).Once()
+
+	h := &Handler{auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    `{"name":"Escalated","permissions":[{"action":"delete","resource":"accounts"}]}`,
+	}
+
+	result, err := h.createGroup(ctx, req)
+
+	require.Error(t, err)
+	assert.Nil(t, result)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "a refused grant must map to a client error, not a 500")
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, err.Error(), "delete:accounts")
+}
+
+func TestDeleteGroup_SystemManagedMaps403(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil)
+	mockAuth.grantAdmin()
+
+	refusal := fmt.Errorf("%w: %q is seeded and maintained by migrations", auth.ErrSystemManagedGroup, "Purchaser")
+	mockAuth.On("DeleteGroup", ctx, ceilingGroupID).Return(refusal).Once()
+
+	h := &Handler{auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+	}
+
+	result, err := h.deleteGroup(ctx, req, ceilingGroupID)
+
+	require.Error(t, err)
+	assert.Nil(t, result)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "a system-managed refusal must map to a client error, not a 500")
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, err.Error(), "Purchaser")
+}
+
+// TestUpdateGroup_AdminAPIKeyActorIsSentinel pins the admin-API-key branch:
+// the key has no user row, so the handler must hand the auth service the
+// sentinel the ceiling recognizes. Passing an empty string here would make
+// the ceiling fail closed on every admin-key group write instead.
+func TestUpdateGroup_AdminAPIKeyActorIsSentinel(t *testing.T) {
+	assert.Equal(t, auth.AdminAPIKeyActorID, apiKeyAdminUserID,
+		"the admin-API-key session identity must be the sentinel auth.grantCeilingPermissions recognizes")
+
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	updated := map[string]any{"id": ceilingGroupID}
+	mockAuth.On("UpdateGroupAPI", ctx, apiKeyAdminUserID, ceilingGroupID, mock.Anything).
+		Return(updated, nil).Once()
+
+	h := &Handler{auth: mockAuth, apiKey: "super-secret-admin-key"}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"x-api-key": "super-secret-admin-key"},
+		Body:    `{"name":"Renamed"}`,
+	}
+
+	result, err := h.updateGroup(ctx, req, ceilingGroupID)
+	require.NoError(t, err)
+	assert.Equal(t, updated, result)
+}

internal/api/handler_groups_test.go

@@ -59,7 +59,7 @@ func TestHandler_createGroup_Success(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
-	mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(createdGroup, nil)
+	mockAuth.On("CreateGroupAPI", ctx, adminSession.UserID, mock.Anything).Return(createdGroup, nil)
 
 	handler := &Handler{auth: mockAuth}
 
@@ -122,7 +122,7 @@ func TestHandler_updateGroup_Success(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
-	mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(updatedGroup, nil)
+	mockAuth.On("UpdateGroupAPI", ctx, adminSession.UserID, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(updatedGroup, nil)
 
 	handler := &Handler{auth: mockAuth}
 

internal/api/handler_ri_exchange_test.go

@@ -1218,8 +1218,10 @@ func (m *mockAuthForExchange) UpdateUserAPI(_ context.Context, _, _ string, _ an
 func (m *mockAuthForExchange) DeleteUser(_ context.Context, _ string) error              { return nil }
 func (m *mockAuthForExchange) ListUsersAPI(_ context.Context) (any, error)               { return nil, nil }
 func (m *mockAuthForExchange) ChangePasswordAPI(_ context.Context, _, _, _ string) error { return nil }
-func (m *mockAuthForExchange) CreateGroupAPI(_ context.Context, _ any) (any, error)      { return nil, nil }
-func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _ string, _ any) (any, error) {
+func (m *mockAuthForExchange) CreateGroupAPI(_ context.Context, _ string, _ any) (any, error) {
+	return nil, nil
+}
+func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _, _ string, _ any) (any, error) {
 	return nil, nil
 }
 func (m *mockAuthForExchange) DeleteGroup(_ context.Context, _ string) error        { return nil }

internal/api/handler_router_test.go

@@ -38,7 +38,7 @@ func TestHandler_updateGroup_Error(t *testing.T) {
 	adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
-	mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(nil, assert.AnError)
+	mockAuth.On("UpdateGroupAPI", ctx, adminSession.UserID, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(nil, assert.AnError)
 
 	handler := &Handler{auth: mockAuth}
 
@@ -79,7 +79,7 @@ func TestHandler_createGroup_Error(t *testing.T) {
 	adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
-	mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(nil, assert.AnError)
+	mockAuth.On("CreateGroupAPI", ctx, adminSession.UserID, mock.Anything).Return(nil, assert.AnError)
 
 	handler := &Handler{auth: mockAuth}
 

internal/api/mocks_test.go

@@ -250,13 +250,13 @@ func (m *MockAuthService) MFARegenerateRecoveryCodesAPI(ctx context.Context, use
 }
 
 // Group management mock methods.
-func (m *MockAuthService) CreateGroupAPI(ctx context.Context, req interface{}) (interface{}, error) {
-	args := m.Called(ctx, req)
+func (m *MockAuthService) CreateGroupAPI(ctx context.Context, actorUserID string, req interface{}) (interface{}, error) {
+	args := m.Called(ctx, actorUserID, req)
 	return args.Get(0), args.Error(1)
 }
 
-func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, groupID string, req interface{}) (interface{}, error) {
-	args := m.Called(ctx, groupID, req)
+func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req interface{}) (interface{}, error) {
+	args := m.Called(ctx, actorUserID, groupID, req)
 	return args.Get(0), args.Error(1)
 }
 

internal/api/types.go

@@ -186,8 +186,13 @@ type AuthServiceInterface interface {
 	MFADisableAPI(ctx context.Context, userID, password, codeOrRecovery string) error
 	MFARegenerateRecoveryCodesAPI(ctx context.Context, userID, code string) (recoveryCodes []string, err error)
 	// Group management - uses auth.API* types
-	CreateGroupAPI(ctx context.Context, req any) (any, error)
-	UpdateGroupAPI(ctx context.Context, groupID string, req any) (any, error)
+	// CreateGroupAPI / UpdateGroupAPI take the acting principal so the
+	// service can enforce the grant ceiling: a caller may not write a
+	// permission onto a group that their own effective permissions do not
+	// hold, and the money verbs carved out of admin:* are not grantable at
+	// all (issues #1550, #1629).
+	CreateGroupAPI(ctx context.Context, actorUserID string, req any) (any, error)
+	UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req any) (any, error)
 	DeleteGroup(ctx context.Context, groupID string) error
 	GetGroupAPI(ctx context.Context, groupID string) (any, error)
 	ListGroupsAPI(ctx context.Context) (any, error)

internal/auth/errors.go

@@ -34,6 +34,35 @@ var (
 	// permission. Mapped to 403 (issue #907).
 	ErrSelfEscalation = errors.New("cannot escalate your own group membership")
 
+	// ErrPermissionCeiling is returned by a group write that would grant a
+	// permission the acting user does not themselves hold (or holds only at
+	// a narrower constraint scope), and by the fail-closed paths where the
+	// acting user's own permissions cannot be resolved at all. Mapped to 403
+	// (issue #1550).
+	ErrPermissionCeiling = errors.New("permission ceiling exceeded")
+
+	// ErrPermissionNotGrantable is returned by a group write that would ADD
+	// one of the money-spending verbs carved out of the admin:* wildcard
+	// (adminCarvedOuts). Those are reserved for separation of duties and are
+	// provisioned by migration, never through the API. Mapped to 403
+	// (issues #923, #1550).
+	ErrPermissionNotGrantable = errors.New("permission not grantable")
+
+	// ErrInvalidPermission is returned when a group write carries a
+	// permission entry with a blank action or resource. A blank resource is
+	// malformed input, NOT a request for the "*" wildcard: the group-edit
+	// form renders an empty stored resource as the selected "All (*)" option
+	// and saves it back as view:*, and the same widening is reachable
+	// directly through the API because nothing validated the list. Mapped to
+	// 400 (issues #1730, #1550).
+	ErrInvalidPermission = errors.New("invalid permission")
+
+	// ErrSystemManagedGroup is returned when a write targets one of the
+	// seeded, system-managed groups. Their contents are owned by migrations;
+	// an API edit that reshapes them can silently disable a whole capability
+	// tenant-wide. Mapped to 403 (issue #1629).
+	ErrSystemManagedGroup = errors.New("system-managed group cannot be modified")
+
 	// ErrCurrentPasswordIncorrect is returned by UpdateUserProfile when the
 	// caller-supplied current password does not match the stored hash. Mapped
 	// to 401 at the API layer (the acting user is verifying their own