From 7d75eb5397458597ee2ca50592f6123e0f60b7b8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:48:38 +0000 Subject: [PATCH] chore(api): type AuthServiceInterface user and group writes with the auth.API* request and response types CreateUserAPI, UpdateUserAPI, CreateGroupAPI and UpdateGroupAPI took and returned any, and each service implementation opened with a type assertion that failed with "invalid request type" at runtime. The import cycle that once justified any is gone (api already imports auth), so the interface, auth.Service, the server adapter and every test double now use the concrete request types and *APICreateUserResponse / *APIUser / *APIGroup responses. A wrong request type is now a compile error. No behavior change: the handlers decode into the same structs and return the same values to the same JSON encoder. A new HandleRequest-level test pins the four endpoints' request values and response bytes; it passes unchanged on both sides of this commit. The three adapter tests that passed maps (and so only ever hit the type-assertion error) now send typed requests and assert the store write and response. Closes #169 Co-Authored-By: Claude Opus 5.5 --- internal/api/group_explicit_zero_cap_test.go | 4 +- internal/api/handler_coverage_test.go | 10 +- internal/api/handler_groups_ceiling_test.go | 2 +- internal/api/handler_groups_test.go | 13 +- internal/api/handler_ri_exchange_test.go | 8 +- internal/api/handler_users_test.go | 20 +- ...dler_users_update_race_integration_test.go | 2 +- internal/api/mocks_test.go | 28 +- internal/api/types.go | 8 +- internal/api/user_group_wire_format_test.go | 248 ++++++++++++++++++ internal/auth/service_api.go | 30 +-- internal/auth/service_api_test.go | 94 ++----- internal/server/adapter_test.go | 56 ++-- internal/server/app.go | 8 +- 14 files changed, 369 insertions(+), 162 deletions(-) create mode 100644 internal/api/user_group_wire_format_test.go diff --git a/internal/api/group_explicit_zero_cap_test.go b/internal/api/group_explicit_zero_cap_test.go index a90bbd2d9..7b66a0a94 100644 --- a/internal/api/group_explicit_zero_cap_test.go +++ b/internal/api/group_explicit_zero_cap_test.go @@ -30,11 +30,11 @@ type realGroupAuth struct { svc *auth.Service } -func (a *realGroupAuth) CreateGroupAPI(ctx context.Context, actor string, req any) (any, error) { +func (a *realGroupAuth) CreateGroupAPI(ctx context.Context, actor string, req auth.APICreateGroupRequest) (*auth.APIGroup, error) { return a.svc.CreateGroupAPI(ctx, actor, req) } -func (a *realGroupAuth) UpdateGroupAPI(ctx context.Context, actor, id string, req any) (any, error) { +func (a *realGroupAuth) UpdateGroupAPI(ctx context.Context, actor, id string, req auth.APIUpdateGroupRequest) (*auth.APIGroup, error) { return a.svc.UpdateGroupAPI(ctx, actor, id, req) } diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index 9dd7077d7..966289f8f 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -482,7 +482,9 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() const groupID = "00000000-0000-5000-8000-000000000005" - created := func() map[string]string { return map[string]string{"id": "new-user"} } + created := func() *auth.APICreateUserResponse { + return &auth.APICreateUserResponse{APIUser: &auth.APIUser{ID: "new-user"}} + } // The actor is the session user (grant ceiling, #226) and the password // arrives decoded. mockAuth.On("CreateUserAPI", ctx, "admin", auth.APICreateUserRequest{ @@ -528,7 +530,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth := strictAuthMock(t) mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() - updated := func() map[string]string { return map[string]string{"id": testID, "email": "updated@example.com"} } + updated := func() *auth.APIUser { return &auth.APIUser{ID: testID, Email: "updated@example.com"} } mockAuth.On("UpdateUserAPI", ctx, "admin", testID, auth.APIUpdateUserRequest{Email: "updated@example.com"}).Return(updated(), nil) h := &Handler{auth: mockAuth} @@ -568,7 +570,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth := strictAuthMock(t) mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() - created := func() map[string]string { return map[string]string{"id": "new-group"} } + created := func() *auth.APIGroup { return &auth.APIGroup{ID: "new-group"} } mockAuth.On("CreateGroupAPI", ctx, "admin", auth.APICreateGroupRequest{Name: "New Group"}).Return(created(), nil) h := &Handler{auth: mockAuth} @@ -608,7 +610,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth := strictAuthMock(t) mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() - updated := func() map[string]string { return map[string]string{"id": testID, "name": "Updated Group"} } + updated := func() *auth.APIGroup { return &auth.APIGroup{ID: testID, Name: "Updated Group"} } mockAuth.On("UpdateGroupAPI", ctx, "admin", testID, auth.APIUpdateGroupRequest{Name: "Updated Group"}).Return(updated(), nil) h := &Handler{auth: mockAuth} diff --git a/internal/api/handler_groups_ceiling_test.go b/internal/api/handler_groups_ceiling_test.go index 091ee30cf..5a88e86cd 100644 --- a/internal/api/handler_groups_ceiling_test.go +++ b/internal/api/handler_groups_ceiling_test.go @@ -124,7 +124,7 @@ func TestUpdateGroup_AdminAPIKeyActorIsSentinel(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) - updated := map[string]any{"id": ceilingGroupID} + updated := &auth.APIGroup{ID: ceilingGroupID} mockAuth.On("UpdateGroupAPI", ctx, apiKeyAdminUserID, ceilingGroupID, mock.Anything). Return(updated, nil).Once() diff --git a/internal/api/handler_groups_test.go b/internal/api/handler_groups_test.go index e2aa18ed6..5021cbeed 100644 --- a/internal/api/handler_groups_test.go +++ b/internal/api/handler_groups_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -52,9 +53,9 @@ func TestHandler_createGroup_Success(t *testing.T) { Email: "admin@example.com", } - createdGroup := map[string]interface{}{ - "id": "33333333-3333-3333-3333-333333333333", - "name": "New Group", + createdGroup := &auth.APIGroup{ + ID: "33333333-3333-3333-3333-333333333333", + Name: "New Group", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) @@ -115,9 +116,9 @@ func TestHandler_updateGroup_Success(t *testing.T) { Email: "admin@example.com", } - updatedGroup := map[string]interface{}{ - "id": "11111111-1111-1111-1111-111111111111", - "name": "Updated Group", + updatedGroup := &auth.APIGroup{ + ID: "11111111-1111-1111-1111-111111111111", + Name: "Updated Group", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 51d463a8e..a17f17352 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1615,19 +1615,19 @@ func (m *mockAuthForExchange) GetUser(_ context.Context, _ string) (*User, error func (m *mockAuthForExchange) UpdateUserProfile(_ context.Context, _, _, _, _ string) error { return nil } -func (m *mockAuthForExchange) CreateUserAPI(_ context.Context, _ string, _ any) (any, error) { +func (m *mockAuthForExchange) CreateUserAPI(_ context.Context, _ string, _ auth.APICreateUserRequest) (*auth.APICreateUserResponse, error) { return nil, nil } -func (m *mockAuthForExchange) UpdateUserAPI(_ context.Context, _, _ string, _ any) (any, error) { +func (m *mockAuthForExchange) UpdateUserAPI(_ context.Context, _, _ string, _ auth.APIUpdateUserRequest) (*auth.APIUser, error) { return nil, nil } 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, _ string, _ any) (any, error) { +func (m *mockAuthForExchange) CreateGroupAPI(_ context.Context, _ string, _ auth.APICreateGroupRequest) (*auth.APIGroup, error) { return nil, nil } -func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _, _ string, _ any) (any, error) { +func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _, _ string, _ auth.APIUpdateGroupRequest) (*auth.APIGroup, error) { return nil, nil } func (m *mockAuthForExchange) DeleteGroup(_ context.Context, _ string) error { return nil } diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index 231d69e48..200aab68e 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -80,11 +80,11 @@ func TestHandler_createUser_Success(t *testing.T) { Email: "admin@example.com", } - createdUser := &auth.APIUser{ + createdUser := &auth.APICreateUserResponse{APIUser: &auth.APIUser{ ID: "33333333-3333-3333-3333-333333333333", Email: "newuser@example.com", Groups: []string{"00000000-0000-5000-8000-000000000005"}, - } + }} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() @@ -148,10 +148,10 @@ func TestHandler_updateUser_Success(t *testing.T) { Email: "admin@example.com", } - updatedUser := map[string]interface{}{ - "id": "11111111-1111-1111-1111-111111111111", - "email": "user@example.com", - "groups": []string{"00000000-0000-5000-8000-000000000001"}, + updatedUser := &auth.APIUser{ + ID: "11111111-1111-1111-1111-111111111111", + Email: "user@example.com", + Groups: []string{"00000000-0000-5000-8000-000000000001"}, } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) @@ -189,10 +189,10 @@ func TestHandler_updateUser_Deactivate(t *testing.T) { Email: "admin@example.com", } - updatedUser := map[string]interface{}{ - "id": "11111111-1111-1111-1111-111111111111", - "email": "user@example.com", - "active": false, + updatedUser := &auth.APIUser{ + ID: "11111111-1111-1111-1111-111111111111", + Email: "user@example.com", + Active: false, } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) diff --git a/internal/api/handler_users_update_race_integration_test.go b/internal/api/handler_users_update_race_integration_test.go index 7d7f3b819..37e7c7f1d 100644 --- a/internal/api/handler_users_update_race_integration_test.go +++ b/internal/api/handler_users_update_race_integration_test.go @@ -24,7 +24,7 @@ type adminUserUpdateAuth struct { marketplaceAuthFixture } -func (a *adminUserUpdateAuth) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req any) (any, error) { +func (a *adminUserUpdateAuth) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req auth.APIUpdateUserRequest) (*auth.APIUser, error) { return a.service.UpdateUserAPI(ctx, actorUserID, userID, req) } diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 2851eb04c..8606dd89b 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -250,14 +250,20 @@ func (m *MockAuthService) UpdateUserProfile(ctx context.Context, userID string, } // User management mock methods. -func (m *MockAuthService) CreateUserAPI(ctx context.Context, actorUserID string, req interface{}) (interface{}, error) { +func (m *MockAuthService) CreateUserAPI(ctx context.Context, actorUserID string, req auth.APICreateUserRequest) (*auth.APICreateUserResponse, error) { args := m.Called(ctx, actorUserID, req) - return args.Get(0), args.Error(1) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*auth.APICreateUserResponse), args.Error(1) } -func (m *MockAuthService) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req interface{}) (interface{}, error) { +func (m *MockAuthService) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req auth.APIUpdateUserRequest) (*auth.APIUser, error) { args := m.Called(ctx, actorUserID, userID, req) - return args.Get(0), args.Error(1) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*auth.APIUser), args.Error(1) } func (m *MockAuthService) DeleteUser(ctx context.Context, userID string) error { @@ -303,14 +309,20 @@ func (m *MockAuthService) MFARegenerateRecoveryCodesAPI(ctx context.Context, use } // Group management mock methods. -func (m *MockAuthService) CreateGroupAPI(ctx context.Context, actorUserID string, req interface{}) (interface{}, error) { +func (m *MockAuthService) CreateGroupAPI(ctx context.Context, actorUserID string, req auth.APICreateGroupRequest) (*auth.APIGroup, error) { args := m.Called(ctx, actorUserID, req) - return args.Get(0), args.Error(1) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*auth.APIGroup), args.Error(1) } -func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req interface{}) (interface{}, error) { +func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req auth.APIUpdateGroupRequest) (*auth.APIGroup, error) { args := m.Called(ctx, actorUserID, groupID, req) - return args.Get(0), args.Error(1) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*auth.APIGroup), args.Error(1) } func (m *MockAuthService) DeleteGroup(ctx context.Context, groupID string) error { diff --git a/internal/api/types.go b/internal/api/types.go index 9ff2a91f1..a17bb5088 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -199,8 +199,8 @@ type AuthServiceInterface interface { GetUser(ctx context.Context, userID string) (*User, error) UpdateUserProfile(ctx context.Context, userID string, email string, currentPassword string, newPassword string) error // User management - uses auth.API* types - CreateUserAPI(ctx context.Context, actorUserID string, req any) (any, error) - UpdateUserAPI(ctx context.Context, actorUserID, userID string, req any) (any, error) + CreateUserAPI(ctx context.Context, actorUserID string, req auth.APICreateUserRequest) (*auth.APICreateUserResponse, error) + UpdateUserAPI(ctx context.Context, actorUserID, userID string, req auth.APIUpdateUserRequest) (*auth.APIUser, error) DeleteUser(ctx context.Context, userID string) error ListUsersAPI(ctx context.Context) (any, error) ChangePasswordAPI(ctx context.Context, userID, currentPassword, newPassword string) error @@ -217,8 +217,8 @@ type AuthServiceInterface interface { // 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) + CreateGroupAPI(ctx context.Context, actorUserID string, req auth.APICreateGroupRequest) (*auth.APIGroup, error) + UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req auth.APIUpdateGroupRequest) (*auth.APIGroup, error) DeleteGroup(ctx context.Context, groupID string) error GetGroupAPI(ctx context.Context, groupID string) (any, error) ListGroupsAPI(ctx context.Context) (any, error) diff --git a/internal/api/user_group_wire_format_test.go b/internal/api/user_group_wire_format_test.go new file mode 100644 index 000000000..32050adc1 --- /dev/null +++ b/internal/api/user_group_wire_format_test.go @@ -0,0 +1,248 @@ +package api + +// Issue #169 pins the user and group write endpoints' wire format across the +// AuthServiceInterface refactor that replaced the any request and response +// types with the concrete auth.API* types. Each case drives the full +// HandleRequest path (JSON decode, router, handler, interface call, JSON +// encode), asserts the exact request value that crosses the interface, and +// compares the response body byte for byte against a literal recorded on the +// pre-refactor code. The test source is identical before and after the +// refactor, so a pass on both sides shows the wire format did not move. + +import ( + "context" + "encoding/base64" + "encoding/json" + "reflect" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const ( + wireActorID = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" + wireUserID = "11111111-1111-4111-8111-111111111111" + wireGroupID = "22222222-2222-4222-8222-222222222222" + wireGroupIDB = "33333333-3333-4333-8333-333333333333" + wireAccountID = "2f1c8e40-0000-4000-8000-000000000169" + wireToken = "session-169" + wireCSRF = "csrf-169" + wirePassword = "Wire-169-pass" +) + +type wireCase struct { + name string + method string + path string + body string + mockName string + // args are the expected leading interface arguments after ctx and + // before the request value. + args []any + wantReq any + response any + wantBody string +} + +// wireDecodedConstraint decodes a constraint fragment the way the handler +// does, so the expected request carries the unexported explicit-max_amount +// marker that only UnmarshalJSON sets (issue #133). Comparing against it +// pins that the marker survives the interface call. +func wireDecodedConstraint(t *testing.T, fragment string) *auth.APIPermissionConstraint { + t.Helper() + c := new(auth.APIPermissionConstraint) + require.NoError(t, json.Unmarshal([]byte(fragment), c)) + return c +} + +const wireConstraintJSON = `{"accounts":["` + wireAccountID + `"],"providers":["aws"],"services":["ec2"],"regions":["us-east-1"],"max_amount":250.5}` + +func wireFormatCases(t *testing.T) []wireCase { + t.Helper() + inviteSent := false + inactive := false + return []wireCase{ + { + name: "POST /api/users", + method: "POST", + path: "/api/users", + body: `{"email":"new@example.com","password":"` + base64.StdEncoding.EncodeToString([]byte(wirePassword)) + `","groups":["` + wireGroupID + `"]}`, + mockName: "CreateUserAPI", + args: []any{wireActorID}, + wantReq: auth.APICreateUserRequest{ + Email: "new@example.com", + Password: wirePassword, + Groups: []string{wireGroupID}, + }, + response: &auth.APICreateUserResponse{ + APIUser: &auth.APIUser{ + ID: wireUserID, + Email: "new@example.com", + CreatedAt: "2026-01-02T03:04:05Z", + UpdatedAt: "2026-01-02T03:04:05Z", + Groups: []string{wireGroupID}, + Active: true, + }, + InviteEmailSent: &inviteSent, + InviteEmailError: "smtp down", + }, + wantBody: `{"id":"11111111-1111-4111-8111-111111111111","email":"new@example.com","created_at":"2026-01-02T03:04:05Z","updated_at":"2026-01-02T03:04:05Z","groups":["22222222-2222-4222-8222-222222222222"],"mfa_enabled":false,"active":true,"invite_email_sent":false,"invite_email_error":"smtp down"}`, + }, + { + name: "PUT /api/users/{id}", + method: "PUT", + path: "/api/users/" + wireUserID, + body: `{"email":"renamed@example.com","groups":["` + wireGroupID + `","` + wireGroupIDB + `"],"active":false}`, + mockName: "UpdateUserAPI", + args: []any{wireActorID, wireUserID}, + wantReq: auth.APIUpdateUserRequest{ + Email: "renamed@example.com", + Groups: []string{wireGroupID, wireGroupIDB}, + Active: &inactive, + }, + response: &auth.APIUser{ + ID: wireUserID, + Email: "renamed@example.com", + CreatedAt: "2026-01-02T03:04:05Z", + UpdatedAt: "2026-02-03T04:05:06Z", + LastLogin: "2026-02-01T00:00:00Z", + Groups: []string{wireGroupID, wireGroupIDB}, + MFAEnabled: true, + Active: false, + }, + wantBody: `{"id":"11111111-1111-4111-8111-111111111111","email":"renamed@example.com","created_at":"2026-01-02T03:04:05Z","updated_at":"2026-02-03T04:05:06Z","last_login":"2026-02-01T00:00:00Z","groups":["22222222-2222-4222-8222-222222222222","33333333-3333-4333-8333-333333333333"],"mfa_enabled":true,"active":false}`, + }, + { + name: "POST /api/groups", + method: "POST", + path: "/api/groups", + body: `{"name":"Ops","description":"Operators","permissions":[{"action":"view","resource":"plans","constraints":` + wireConstraintJSON + `}],"allowed_accounts":["` + wireAccountID + `"]}`, + mockName: "CreateGroupAPI", + args: []any{wireActorID}, + wantReq: auth.APICreateGroupRequest{ + Name: "Ops", + Description: "Operators", + Permissions: []auth.APIPermission{{ + Action: "view", + Resource: "plans", + Constraints: wireDecodedConstraint(t, wireConstraintJSON), + }}, + AllowedAccounts: []string{wireAccountID}, + }, + response: &auth.APIGroup{ + ID: wireGroupID, + Name: "Ops", + Description: "Operators", + CreatedAt: "2026-01-02T03:04:05Z", + UpdatedAt: "2026-01-02T03:04:05Z", + Permissions: []auth.APIPermission{{ + Action: "view", + Resource: "plans", + Constraints: &auth.APIPermissionConstraint{ + Accounts: []string{wireAccountID}, + Providers: []string{"aws"}, + Services: []string{"ec2"}, + Regions: []string{"us-east-1"}, + MaxAmount: 250.5, + }, + }}, + AllowedAccounts: []string{wireAccountID}, + }, + wantBody: `{"id":"22222222-2222-4222-8222-222222222222","name":"Ops","description":"Operators","created_at":"2026-01-02T03:04:05Z","updated_at":"2026-01-02T03:04:05Z","permissions":[{"constraints":{"accounts":["2f1c8e40-0000-4000-8000-000000000169"],"providers":["aws"],"services":["ec2"],"regions":["us-east-1"],"max_amount":250.5},"action":"view","resource":"plans"}],"allowed_accounts":["2f1c8e40-0000-4000-8000-000000000169"]}`, + }, + { + // An explicit empty allowed_accounts must reach the service as a + // non-nil empty slice (it clears the restriction), and omitted + // permissions as nil ("not sent"). + name: "PUT /api/groups/{id}", + method: "PUT", + path: "/api/groups/" + wireGroupID, + body: `{"name":"Ops 2","allowed_accounts":[]}`, + mockName: "UpdateGroupAPI", + args: []any{wireActorID, wireGroupID}, + wantReq: auth.APIUpdateGroupRequest{ + Name: "Ops 2", + AllowedAccounts: []string{}, + }, + response: &auth.APIGroup{ + ID: wireGroupID, + Name: "Ops 2", + CreatedAt: "2026-01-02T03:04:05Z", + UpdatedAt: "2026-02-03T04:05:06Z", + Permissions: []auth.APIPermission{{Action: "view", Resource: "plans"}}, + AllowedAccounts: []string{}, + }, + wantBody: `{"id":"22222222-2222-4222-8222-222222222222","name":"Ops 2","created_at":"2026-01-02T03:04:05Z","updated_at":"2026-02-03T04:05:06Z","permissions":[{"action":"view","resource":"plans"}],"allowed_accounts":[]}`, + }, + } +} + +func TestUserGroupWrites_WireFormat(t *testing.T) { + for _, tc := range wireFormatCases(t) { + t.Run(tc.name, func(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, wireToken). + Return(&Session{UserID: wireActorID, Email: "admin@example.com"}, nil) + mockAuth.On("ValidateCSRFToken", mock.Anything, wireToken, wireCSRF).Return(nil) + mockAuth.grantAdmin() + + var gotReq any + callArgs := append([]any{mock.Anything}, tc.args...) + callArgs = append(callArgs, mock.Anything) + reqIdx := len(callArgs) - 1 + mockAuth.On(tc.mockName, callArgs...). + Run(func(args mock.Arguments) { gotReq = args.Get(reqIdx) }). + Return(tc.response, nil).Once() + + h := &Handler{auth: mockAuth} + resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer " + wireToken, + "Content-Type": "application/json", + "X-CSRF-Token": wireCSRF, + }, + Body: tc.body, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: tc.method, Path: tc.path}, + }, + }) + require.NoError(t, err) + require.NotNil(t, resp) + + require.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) + assert.Equal(t, tc.wantReq, gotReq, "request value crossing AuthServiceInterface") + assert.Equal(t, tc.wantBody, resp.Body, "response body bytes") + mockAuth.AssertExpectations(t) + }) + } +} + +// TestAuthServiceInterface_UserGroupWritesAreTyped pins the four user and +// group write methods to their concrete request and response types, so a +// wrong request type is a compile error instead of the runtime "invalid +// request type" the any signatures allowed (issue #169). +func TestAuthServiceInterface_UserGroupWritesAreTyped(t *testing.T) { + iface := reflect.TypeOf((*AuthServiceInterface)(nil)).Elem() + cases := []struct { + method string + req reflect.Type + resp reflect.Type + }{ + {"CreateUserAPI", reflect.TypeOf(auth.APICreateUserRequest{}), reflect.TypeOf(&auth.APICreateUserResponse{})}, + {"UpdateUserAPI", reflect.TypeOf(auth.APIUpdateUserRequest{}), reflect.TypeOf(&auth.APIUser{})}, + {"CreateGroupAPI", reflect.TypeOf(auth.APICreateGroupRequest{}), reflect.TypeOf(&auth.APIGroup{})}, + {"UpdateGroupAPI", reflect.TypeOf(auth.APIUpdateGroupRequest{}), reflect.TypeOf(&auth.APIGroup{})}, + } + for _, tc := range cases { + m, ok := iface.MethodByName(tc.method) + require.True(t, ok, tc.method) + in := m.Type.In(m.Type.NumIn() - 1) + assert.Equal(t, tc.req.String(), in.String(), "%s request parameter type", tc.method) + require.Equal(t, 2, m.Type.NumOut(), tc.method) + assert.Equal(t, tc.resp.String(), m.Type.Out(0).String(), "%s response type", tc.method) + } +} diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index bcdfa753b..0504f83ad 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -235,18 +235,16 @@ func apiPermissionToPermission(ap APIPermission) Permission { return p } -// API adapter methods - these implement the AuthServiceInterface from handler.go -// They use any to avoid import cycles with the api package +// API adapter methods - these back api.AuthServiceInterface via the server +// package's authServiceAdapter. The api package imports auth, so the user and +// group writes take and return the concrete API* types directly (issue #169). +// The methods that still use any predate that and are not yet converted. // CreateUserAPI creates a new user via the API. actorUserID is the // authenticated caller performing the creation (from the session, never the // request body); it is used by the service layer to enforce the membership // grant ceiling (issue #226). -func (s *Service) CreateUserAPI(ctx context.Context, actorUserID string, reqInterface any) (any, error) { - req, ok := reqInterface.(APICreateUserRequest) - if !ok { - return nil, fmt.Errorf("invalid request type") - } +func (s *Service) CreateUserAPI(ctx context.Context, actorUserID string, req APICreateUserRequest) (*APICreateUserResponse, error) { authReq := CreateUserRequest{ Email: req.Email, Password: req.Password, @@ -266,11 +264,7 @@ func (s *Service) CreateUserAPI(ctx context.Context, actorUserID string, reqInte // UpdateUserAPI updates a user via the API. actorUserID is the authenticated // caller performing the change (from the session, never the request body); it // is used by the service layer to enforce the self-escalation guard (#907). -func (s *Service) UpdateUserAPI(ctx context.Context, actorUserID, userID string, reqInterface any) (any, error) { - req, ok := reqInterface.(APIUpdateUserRequest) - if !ok { - return nil, fmt.Errorf("invalid request type") - } +func (s *Service) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req APIUpdateUserRequest) (*APIUser, error) { authReq := UpdateUserRequest{ GroupIDs: req.Groups, } @@ -333,11 +327,7 @@ func apiPermissionsToPermissions(in []APIPermission) ([]Permission, error) { // principal (a user ID, or AdminAPIKeyActorID for the stateless admin API // key); the requested permissions are checked against that principal's own // effective set before anything is written (issue #1550). -func (s *Service) CreateGroupAPI(ctx context.Context, actorUserID string, reqInterface any) (any, error) { - req, ok := reqInterface.(APICreateGroupRequest) - if !ok { - return nil, fmt.Errorf("invalid request type") - } +func (s *Service) CreateGroupAPI(ctx context.Context, actorUserID string, req APICreateGroupRequest) (*APIGroup, error) { perms, err := apiPermissionsToPermissions(req.Permissions) if err != nil { return nil, err @@ -405,11 +395,7 @@ func updateRequestPermissions(req APIUpdateGroupRequest) ([]Permission, error) { // against the acting principal's own effective permissions before writing // (issues #1550, #1629). actorUserID is a user ID, or AdminAPIKeyActorID for // the stateless admin API key. -func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, reqInterface any) (any, error) { - req, ok := reqInterface.(APIUpdateGroupRequest) - if !ok { - return nil, fmt.Errorf("invalid request type") - } +func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req APIUpdateGroupRequest) (*APIGroup, error) { group, err := s.GetGroup(ctx, groupID) if err != nil { return nil, err diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index 984465cdb..0b7352181 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -151,28 +151,15 @@ func TestService_CreateUserAPI(t *testing.T) { require.NoError(t, err) require.NotNil(t, result) - resp, ok := result.(*APICreateUserResponse) - require.True(t, ok, "CreateUserAPI should wrap the response in APICreateUserResponse") - require.NotNil(t, resp.APIUser) - assert.Equal(t, "newuser@example.com", resp.Email) - assert.Equal(t, []string{"group-1"}, resp.Groups) + require.NotNil(t, result.APIUser) + assert.Equal(t, "newuser@example.com", result.Email) + assert.Equal(t, []string{"group-1"}, result.Groups) // Non-invite path: no invite-email status fields. - assert.Nil(t, resp.InviteEmailSent) - assert.Empty(t, resp.InviteEmailError) + assert.Nil(t, result.InviteEmailSent) + assert.Empty(t, result.InviteEmailError) mockStore.AssertExpectations(t) }) - - t.Run("invalid request type", func(t *testing.T) { - mockStore := new(MockStore) - mockEmail := new(MockEmailSender) - service := createTestService(mockStore, mockEmail) - - result, err := service.CreateUserAPI(ctx, "", "invalid") - assert.Error(t, err) - assert.Nil(t, result) - assert.Contains(t, err.Error(), "invalid request type") - }) } func TestService_UpdateUserAPI(t *testing.T) { @@ -201,9 +188,7 @@ func TestService_UpdateUserAPI(t *testing.T) { require.NoError(t, err) assert.NotNil(t, result) - apiUser, ok := result.(*APIUser) - assert.True(t, ok) - assert.Equal(t, []string{"group-2"}, apiUser.Groups) + assert.Equal(t, []string{"group-2"}, result.Groups) mockStore.AssertExpectations(t) }) @@ -244,9 +229,7 @@ func TestService_UpdateUserAPI(t *testing.T) { require.NoError(t, err) require.NotNil(t, result) - apiUser, ok := result.(*APIUser) - require.True(t, ok) - assert.Equal(t, "new@example.com", apiUser.Email, + assert.Equal(t, "new@example.com", result.Email, "returned APIUser must reflect the new email; the frontend's success toast keys off this response") mockStore.AssertExpectations(t) @@ -282,17 +265,6 @@ func TestService_UpdateUserAPI(t *testing.T) { mockStore.AssertExpectations(t) }) - t.Run("invalid request type", func(t *testing.T) { - mockStore := new(MockStore) - mockEmail := new(MockEmailSender) - service := createTestService(mockStore, mockEmail) - - result, err := service.UpdateUserAPI(ctx, "", "user-123", "invalid") - assert.Error(t, err) - assert.Nil(t, result) - assert.Contains(t, err.Error(), "invalid request type") - }) - // Regression for issue #89: before this fix, APIUpdateUserRequest had no // Active field at all, so a PUT /api/users/{id} body could never // deactivate a user -- the same field-silently-dropped defect class @@ -325,9 +297,7 @@ func TestService_UpdateUserAPI(t *testing.T) { require.NoError(t, err) require.NotNil(t, result) - apiUser, ok := result.(*APIUser) - require.True(t, ok) - assert.False(t, apiUser.Active, "the API response must reflect the deactivation") + assert.False(t, result.Active, "the API response must reflect the deactivation") mockStore.AssertExpectations(t) }) @@ -355,9 +325,8 @@ func TestService_UpdateUserAPI(t *testing.T) { result, err := service.UpdateUserAPI(ctx, "", "user-123", req) require.NoError(t, err) - apiUser, ok := result.(*APIUser) - require.True(t, ok) - assert.True(t, apiUser.Active) + require.NotNil(t, result) + assert.True(t, result.Active) mockStore.AssertExpectations(t) }) @@ -454,25 +423,12 @@ func TestService_CreateGroupAPI(t *testing.T) { require.NoError(t, err) assert.NotNil(t, result) - apiGroup, ok := result.(*APIGroup) - assert.True(t, ok) - assert.Equal(t, "Test Group", apiGroup.Name) - assert.Equal(t, "Test description", apiGroup.Description) - assert.Len(t, apiGroup.Permissions, 1) + assert.Equal(t, "Test Group", result.Name) + assert.Equal(t, "Test description", result.Description) + assert.Len(t, result.Permissions, 1) mockStore.AssertExpectations(t) }) - - t.Run("invalid request type", func(t *testing.T) { - mockStore := new(MockStore) - mockEmail := new(MockEmailSender) - service := createTestService(mockStore, mockEmail) - - result, err := service.CreateGroupAPI(ctx, AdminAPIKeyActorID, "invalid") - assert.Error(t, err) - assert.Nil(t, result) - assert.Contains(t, err.Error(), "invalid request type") - }) } func TestService_UpdateGroupAPI(t *testing.T) { @@ -540,9 +496,8 @@ func TestService_UpdateGroupAPI(t *testing.T) { result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) require.NoError(t, err) - apiGroup, ok := result.(*APIGroup) - require.True(t, ok) - assert.Equal(t, "New description", apiGroup.Description) + require.NotNil(t, result) + assert.Equal(t, "New description", result.Description) assert.Equal(t, []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, existingGroup.Permissions, "omitting the field must leave permissions untouched") mockStore.AssertExpectations(t) @@ -578,26 +533,13 @@ func TestService_UpdateGroupAPI(t *testing.T) { require.NoError(t, err) assert.NotNil(t, result) - apiGroup, ok := result.(*APIGroup) - assert.True(t, ok) - assert.Equal(t, "New Name", apiGroup.Name) - assert.Equal(t, "New description", apiGroup.Description) - assert.Len(t, apiGroup.Permissions, 1) + assert.Equal(t, "New Name", result.Name) + assert.Equal(t, "New description", result.Description) + assert.Len(t, result.Permissions, 1) mockStore.AssertExpectations(t) }) - t.Run("invalid request type", func(t *testing.T) { - mockStore := new(MockStore) - mockEmail := new(MockEmailSender) - service := createTestService(mockStore, mockEmail) - - result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", "invalid") - assert.Error(t, err) - assert.Nil(t, result) - assert.Contains(t, err.Error(), "invalid request type") - }) - t.Run("group not found", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender) diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index a1dfd4fee..003b3eb77 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -311,17 +311,22 @@ func TestAuthServiceAdapter_CreateUserAPI(t *testing.T) { adapter, mockStore := createMockAuthService(t) ctx := context.Background() - mockStore.On("GetUserByEmail", ctx, mock.AnythingOfType("string")).Return(nil, nil) - mockStore.On("CreateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil) - - // Pass a properly typed request - _, err := adapter.CreateUserAPI(ctx, "", map[string]interface{}{ - "email": "new@example.com", - "password": "StrongPassword123!", - "groups": []string{"group-viewer"}, + mockStore.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil).Once() + mockStore.On("CreateUser", ctx, mock.MatchedBy(func(u *auth.User) bool { + return u.Email == "new@example.com" && len(u.GroupIDs) == 1 && u.GroupIDs[0] == "group-viewer" + })).Return(nil).Once() + + resp, err := adapter.CreateUserAPI(ctx, "", auth.APICreateUserRequest{ + Email: "new@example.com", + Password: "StrongPassword123!", + Groups: []string{"group-viewer"}, }) - // May fail depending on validation but exercises the code - _ = err + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.APIUser) + assert.Equal(t, "new@example.com", resp.Email) + assert.Equal(t, []string{"group-viewer"}, resp.Groups) + mockStore.AssertExpectations(t) } func TestAuthServiceAdapter_UpdateUserAPI(t *testing.T) { @@ -368,13 +373,19 @@ func TestAuthServiceAdapter_CreateGroupAPI(t *testing.T) { adapter, mockStore := createMockAuthService(t) ctx := context.Background() - mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil) + mockStore.On("CreateGroup", ctx, mock.MatchedBy(func(g *auth.Group) bool { + return g.Name == "test-group" && len(g.Permissions) == 1 + })).Return(nil).Once() - _, err := adapter.CreateGroupAPI(ctx, auth.AdminAPIKeyActorID, map[string]interface{}{ - "name": "test-group", - "permissions": []string{"read:config"}, + group, err := adapter.CreateGroupAPI(ctx, auth.AdminAPIKeyActorID, auth.APICreateGroupRequest{ + Name: "test-group", + Permissions: []auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceConfig}}, }) - _ = err + require.NoError(t, err) + require.NotNil(t, group) + assert.Equal(t, "test-group", group.Name) + assert.Equal(t, []auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceConfig}}, group.Permissions) + mockStore.AssertExpectations(t) } func TestAuthServiceAdapter_UpdateGroupAPI(t *testing.T) { @@ -384,13 +395,18 @@ func TestAuthServiceAdapter_UpdateGroupAPI(t *testing.T) { mockStore.On("GetGroup", ctx, "group-1").Return(&auth.Group{ ID: "group-1", Name: "old-name", - }, nil) - mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil) + }, nil).Once() + mockStore.On("UpdateGroup", ctx, mock.MatchedBy(func(g *auth.Group) bool { + return g.ID == "group-1" && g.Name == "new-name" + })).Return(nil).Once() - _, err := adapter.UpdateGroupAPI(ctx, auth.AdminAPIKeyActorID, "group-1", map[string]interface{}{ - "name": "new-name", + group, err := adapter.UpdateGroupAPI(ctx, auth.AdminAPIKeyActorID, "group-1", auth.APIUpdateGroupRequest{ + Name: "new-name", }) - _ = err + require.NoError(t, err) + require.NotNil(t, group) + assert.Equal(t, "new-name", group.Name) + mockStore.AssertExpectations(t) } func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) { diff --git a/internal/server/app.go b/internal/server/app.go index fd92d96bf..5ec9c0894 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1173,11 +1173,11 @@ func (a *authServiceAdapter) UpdateUserProfile(ctx context.Context, userID, emai } // User management methods - delegate to auth service API methods. -func (a *authServiceAdapter) CreateUserAPI(ctx context.Context, actorUserID string, req any) (any, error) { +func (a *authServiceAdapter) CreateUserAPI(ctx context.Context, actorUserID string, req auth.APICreateUserRequest) (*auth.APICreateUserResponse, error) { return a.service.CreateUserAPI(ctx, actorUserID, req) } -func (a *authServiceAdapter) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req any) (any, error) { +func (a *authServiceAdapter) UpdateUserAPI(ctx context.Context, actorUserID, userID string, req auth.APIUpdateUserRequest) (*auth.APIUser, error) { return a.service.UpdateUserAPI(ctx, actorUserID, userID, req) } @@ -1211,11 +1211,11 @@ func (a *authServiceAdapter) MFARegenerateRecoveryCodesAPI(ctx context.Context, } // Group management methods - delegate to auth service API methods. -func (a *authServiceAdapter) CreateGroupAPI(ctx context.Context, actorUserID string, req any) (any, error) { +func (a *authServiceAdapter) CreateGroupAPI(ctx context.Context, actorUserID string, req auth.APICreateGroupRequest) (*auth.APIGroup, error) { return a.service.CreateGroupAPI(ctx, actorUserID, req) } -func (a *authServiceAdapter) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req any) (any, error) { +func (a *authServiceAdapter) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req auth.APIUpdateGroupRequest) (*auth.APIGroup, error) { return a.service.UpdateGroupAPI(ctx, actorUserID, groupID, req) }