diff --git a/internal/api/group_explicit_zero_cap_test.go b/internal/api/group_explicit_zero_cap_test.go index a90bbd2d..7b66a0a9 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 9dd7077d..966289f8 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 091ee30c..5a88e86c 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 e2aa18ed..5021cbee 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 51d463a8..a17f1735 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 231d69e4..200aab68 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 7d7f3b81..37e7c7f1 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 2851eb04..8606dd89 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 9ff2a91f..a17bb508 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 00000000..32050adc --- /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 bcdfa753..0504f83a 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 984465cd..0b735218 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 a1dfd4fe..003b3eb7 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 fd92d96b..5ec9c089 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) }