From 9f72fc63ac8f8c2c9eb4ed7f2440775ca1eda2fd Mon Sep 17 00:00:00 2001 From: Stefan Benz <46600784+stebenz@users.noreply.github.com> Date: Fri, 8 Mar 2024 09:37:23 +0100 Subject: [PATCH] fix: add additional permission tests to user v2 query endpoints (#7382) Add additional permission integration tests to the user v2 query endpoints including some fixes to correctly check the permissions after the data is known which you want to query. --- internal/api/grpc/user/v2/query.go | 12 +- .../grpc/user/v2/query_integration_test.go | 155 +++++++++- .../api/grpc/user/v2/user_integration_test.go | 290 +++++++++++++++++- internal/integration/integration.go | 7 + internal/query/user.go | 2 +- 5 files changed, 451 insertions(+), 15 deletions(-) diff --git a/internal/api/grpc/user/v2/query.go b/internal/api/grpc/user/v2/query.go index 95733152c7..2fa252b24a 100644 --- a/internal/api/grpc/user/v2/query.go +++ b/internal/api/grpc/user/v2/query.go @@ -14,17 +14,15 @@ import ( ) func (s *Server) GetUserByID(ctx context.Context, req *user.GetUserByIDRequest) (_ *user.GetUserByIDResponse, err error) { - ctxData := authz.GetCtxData(ctx) - if ctxData.UserID != req.GetUserId() { - if err := s.checkPermission(ctx, domain.PermissionUserRead, ctxData.OrgID, req.GetUserId()); err != nil { - return nil, err - } - } - resp, err := s.query.GetUserByID(ctx, true, req.GetUserId()) if err != nil { return nil, err } + if authz.GetCtxData(ctx).UserID != req.GetUserId() { + if err := s.checkPermission(ctx, domain.PermissionUserRead, resp.ResourceOwner, req.GetUserId()); err != nil { + return nil, err + } + } return &user.GetUserByIDResponse{ Details: object.DomainToDetailsPb(&domain.ObjectDetails{ Sequence: resp.Sequence, diff --git a/internal/api/grpc/user/v2/query_integration_test.go b/internal/api/grpc/user/v2/query_integration_test.go index 63790d3c86..352bba89a9 100644 --- a/internal/api/grpc/user/v2/query_integration_test.go +++ b/internal/api/grpc/user/v2/query_integration_test.go @@ -5,10 +5,11 @@ package user_test import ( "context" "fmt" - "github.com/stretchr/testify/assert" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/muhlemmer/gu" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/timestamppb" @@ -151,6 +152,158 @@ func TestServer_GetUserByID(t *testing.T) { } } +func TestServer_GetUserByID_Permission(t *testing.T) { + timeNow := time.Now().UTC() + newOrgOwnerEmail := fmt.Sprintf("%d@permission.get.com", timeNow.UnixNano()) + newOrg := Tester.CreateOrganization(IamCTX, fmt.Sprintf("GetHuman%d", time.Now().UnixNano()), newOrgOwnerEmail) + newUserID := newOrg.CreatedAdmins[0].GetUserId() + type args struct { + ctx context.Context + req *user.GetUserByIDRequest + } + tests := []struct { + name string + args args + want *user.GetUserByIDResponse + wantErr bool + }{ + { + name: "System, ok", + args: args{ + SystemCTX, + &user.GetUserByIDRequest{ + Organization: &object.Organization{ + Org: &object.Organization_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + UserId: newUserID, + }, + }, + want: &user.GetUserByIDResponse{ + User: &user.User{ + State: user.UserState_USER_STATE_ACTIVE, + Username: "", + LoginNames: nil, + PreferredLoginName: "", + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "firstname", + FamilyName: "lastname", + NickName: gu.Ptr(""), + DisplayName: gu.Ptr("firstname lastname"), + PreferredLanguage: gu.Ptr("und"), + Gender: user.Gender_GENDER_UNSPECIFIED.Enum(), + AvatarUrl: "", + }, + Email: &user.HumanEmail{ + Email: newOrgOwnerEmail, + }, + Phone: &user.HumanPhone{}, + }, + }, + }, + Details: &object.Details{ + ChangeDate: timestamppb.New(timeNow), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "Instance, ok", + args: args{ + IamCTX, + &user.GetUserByIDRequest{ + Organization: &object.Organization{ + Org: &object.Organization_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + UserId: newUserID, + }, + }, + want: &user.GetUserByIDResponse{ + User: &user.User{ + State: user.UserState_USER_STATE_ACTIVE, + Username: "", + LoginNames: nil, + PreferredLoginName: "", + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "firstname", + FamilyName: "lastname", + NickName: gu.Ptr(""), + DisplayName: gu.Ptr("firstname lastname"), + PreferredLanguage: gu.Ptr("und"), + Gender: user.Gender_GENDER_UNSPECIFIED.Enum(), + AvatarUrl: "", + }, + Email: &user.HumanEmail{ + Email: newOrgOwnerEmail, + }, + Phone: &user.HumanPhone{}, + }, + }, + }, + Details: &object.Details{ + ChangeDate: timestamppb.New(timeNow), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "Org, error", + args: args{ + CTX, + &user.GetUserByIDRequest{ + Organization: &object.Organization{ + Org: &object.Organization_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + UserId: newUserID, + }, + }, + wantErr: true, + }, + { + name: "User, error", + args: args{ + UserCTX, + &user.GetUserByIDRequest{ + Organization: &object.Organization{ + Org: &object.Organization_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + UserId: newUserID, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Client.GetUserByID(tt.args.ctx, tt.args.req) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + tt.want.User.UserId = tt.args.req.GetUserId() + tt.want.User.Username = newOrgOwnerEmail + tt.want.User.PreferredLoginName = newOrgOwnerEmail + tt.want.User.LoginNames = []string{newOrgOwnerEmail} + if human := tt.want.User.GetHuman(); human != nil { + human.Email.Email = newOrgOwnerEmail + } + assert.Equal(t, tt.want.User, got.User) + } + }) + } +} + type userAttr struct { UserID string Username string diff --git a/internal/api/grpc/user/v2/user_integration_test.go b/internal/api/grpc/user/v2/user_integration_test.go index d35261f87c..ef1cf9081e 100644 --- a/internal/api/grpc/user/v2/user_integration_test.go +++ b/internal/api/grpc/user/v2/user_integration_test.go @@ -27,12 +27,13 @@ import ( ) var ( - CTX context.Context - IamCTX context.Context - UserCTX context.Context - ErrCTX context.Context - Tester *integration.Tester - Client user.UserServiceClient + CTX context.Context + IamCTX context.Context + UserCTX context.Context + SystemCTX context.Context + ErrCTX context.Context + Tester *integration.Tester + Client user.UserServiceClient ) func TestMain(m *testing.M) { @@ -45,6 +46,7 @@ func TestMain(m *testing.M) { UserCTX = Tester.WithAuthorization(ctx, integration.Login) IamCTX = Tester.WithAuthorization(ctx, integration.IAMOwner) + SystemCTX = Tester.WithAuthorization(ctx, integration.SystemUser) CTX, ErrCTX = Tester.WithAuthorization(ctx, integration.OrgOwner), errCtx Client = Tester.Client.UserV2 return m.Run() @@ -552,6 +554,199 @@ func TestServer_AddHumanUser(t *testing.T) { } } +func TestServer_AddHumanUser_Permission(t *testing.T) { + newOrgOwnerEmail := fmt.Sprintf("%d@permission.com", time.Now().UnixNano()) + newOrg := Tester.CreateOrganization(IamCTX, fmt.Sprintf("AddHuman%d", time.Now().UnixNano()), newOrgOwnerEmail) + type args struct { + ctx context.Context + req *user.AddHumanUserRequest + } + tests := []struct { + name string + args args + want *user.AddHumanUserResponse + wantErr bool + }{ + { + name: "System, ok", + args: args{ + SystemCTX, + &user.AddHumanUserRequest{ + Organisation: &object.Organisation{ + Org: &object.Organisation_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + Profile: &user.SetHumanProfile{ + GivenName: "Donald", + FamilyName: "Duck", + NickName: gu.Ptr("Dukkie"), + DisplayName: gu.Ptr("Donald Duck"), + PreferredLanguage: gu.Ptr("en"), + Gender: user.Gender_GENDER_DIVERSE.Enum(), + }, + Email: &user.SetHumanEmail{}, + Phone: &user.SetHumanPhone{}, + Metadata: []*user.SetMetadataEntry{ + { + Key: "somekey", + Value: []byte("somevalue"), + }, + }, + PasswordType: &user.AddHumanUserRequest_Password{ + Password: &user.Password{ + Password: "DifficultPW666!", + ChangeRequired: true, + }, + }, + }, + }, + want: &user.AddHumanUserResponse{ + Details: &object.Details{ + ChangeDate: timestamppb.Now(), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "Instance, ok", + args: args{ + IamCTX, + &user.AddHumanUserRequest{ + Organisation: &object.Organisation{ + Org: &object.Organisation_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + Profile: &user.SetHumanProfile{ + GivenName: "Donald", + FamilyName: "Duck", + NickName: gu.Ptr("Dukkie"), + DisplayName: gu.Ptr("Donald Duck"), + PreferredLanguage: gu.Ptr("en"), + Gender: user.Gender_GENDER_DIVERSE.Enum(), + }, + Email: &user.SetHumanEmail{}, + Phone: &user.SetHumanPhone{}, + Metadata: []*user.SetMetadataEntry{ + { + Key: "somekey", + Value: []byte("somevalue"), + }, + }, + PasswordType: &user.AddHumanUserRequest_Password{ + Password: &user.Password{ + Password: "DifficultPW666!", + ChangeRequired: true, + }, + }, + }, + }, + want: &user.AddHumanUserResponse{ + Details: &object.Details{ + ChangeDate: timestamppb.Now(), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "Org, error", + args: args{ + CTX, + &user.AddHumanUserRequest{ + Organisation: &object.Organisation{ + Org: &object.Organisation_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + Profile: &user.SetHumanProfile{ + GivenName: "Donald", + FamilyName: "Duck", + NickName: gu.Ptr("Dukkie"), + DisplayName: gu.Ptr("Donald Duck"), + PreferredLanguage: gu.Ptr("en"), + Gender: user.Gender_GENDER_DIVERSE.Enum(), + }, + Email: &user.SetHumanEmail{}, + Phone: &user.SetHumanPhone{}, + Metadata: []*user.SetMetadataEntry{ + { + Key: "somekey", + Value: []byte("somevalue"), + }, + }, + PasswordType: &user.AddHumanUserRequest_Password{ + Password: &user.Password{ + Password: "DifficultPW666!", + ChangeRequired: true, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "User, error", + args: args{ + UserCTX, + &user.AddHumanUserRequest{ + Organisation: &object.Organisation{ + Org: &object.Organisation_OrgId{ + OrgId: newOrg.GetOrganizationId(), + }, + }, + Profile: &user.SetHumanProfile{ + GivenName: "Donald", + FamilyName: "Duck", + NickName: gu.Ptr("Dukkie"), + DisplayName: gu.Ptr("Donald Duck"), + PreferredLanguage: gu.Ptr("en"), + Gender: user.Gender_GENDER_DIVERSE.Enum(), + }, + Email: &user.SetHumanEmail{}, + Phone: &user.SetHumanPhone{}, + Metadata: []*user.SetMetadataEntry{ + { + Key: "somekey", + Value: []byte("somevalue"), + }, + }, + PasswordType: &user.AddHumanUserRequest_Password{ + Password: &user.Password{ + Password: "DifficultPW666!", + ChangeRequired: true, + }, + }, + }, + }, + wantErr: true, + }, + } + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + userID := fmt.Sprint(time.Now().UnixNano() + int64(i)) + tt.args.req.UserId = &userID + if email := tt.args.req.GetEmail(); email != nil { + email.Email = fmt.Sprintf("%s@me.now", userID) + } + + if tt.want != nil { + tt.want.UserId = userID + } + + got, err := Client.AddHumanUser(tt.args.ctx, tt.args.req) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + assert.Equal(t, tt.want.GetUserId(), got.GetUserId()) + integration.AssertDetails(t, tt.want, got) + }) + } +} + func TestServer_UpdateHumanUser(t *testing.T) { type args struct { ctx context.Context @@ -908,6 +1103,89 @@ func TestServer_UpdateHumanUser(t *testing.T) { } } +func TestServer_UpdateHumanUser_Permission(t *testing.T) { + newOrgOwnerEmail := fmt.Sprintf("%d@permission.update.com", time.Now().UnixNano()) + newOrg := Tester.CreateOrganization(IamCTX, fmt.Sprintf("UpdateHuman%d", time.Now().UnixNano()), newOrgOwnerEmail) + newUserID := newOrg.CreatedAdmins[0].GetUserId() + type args struct { + ctx context.Context + req *user.UpdateHumanUserRequest + } + tests := []struct { + name string + args args + want *user.UpdateHumanUserResponse + wantErr bool + }{ + { + name: "system, ok", + args: args{ + SystemCTX, + &user.UpdateHumanUserRequest{ + UserId: newUserID, + Username: gu.Ptr(fmt.Sprint("system", time.Now().UnixNano()+1)), + }, + }, + want: &user.UpdateHumanUserResponse{ + Details: &object.Details{ + ChangeDate: timestamppb.Now(), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "instance, ok", + args: args{ + IamCTX, + &user.UpdateHumanUserRequest{ + UserId: newUserID, + Username: gu.Ptr(fmt.Sprint("instance", time.Now().UnixNano()+1)), + }, + }, + want: &user.UpdateHumanUserResponse{ + Details: &object.Details{ + ChangeDate: timestamppb.Now(), + ResourceOwner: newOrg.GetOrganizationId(), + }, + }, + }, + { + name: "org, error", + args: args{ + CTX, + &user.UpdateHumanUserRequest{ + UserId: newUserID, + Username: gu.Ptr(fmt.Sprint("org", time.Now().UnixNano()+1)), + }, + }, + wantErr: true, + }, + { + name: "user, error", + args: args{ + UserCTX, + &user.UpdateHumanUserRequest{ + UserId: newUserID, + Username: gu.Ptr(fmt.Sprint("user", time.Now().UnixNano()+1)), + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + got, err := Client.UpdateHumanUser(tt.args.ctx, tt.args.req) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + integration.AssertDetails(t, tt.want, got) + }) + } +} + func TestServer_LockUser(t *testing.T) { type args struct { ctx context.Context diff --git a/internal/integration/integration.go b/internal/integration/integration.go index cf15b284bd..dff41e811e 100644 --- a/internal/integration/integration.go +++ b/internal/integration/integration.go @@ -252,6 +252,13 @@ func (s *Tester) WithInstanceAuthorization(ctx context.Context, u UserType, inst return s.WithAuthorizationToken(ctx, s.Users.Get(instanceID, u).Token) } +func (s *Tester) GetUserID(u UserType) string { + if u == SystemUser { + s.ensureSystemUser() + } + return s.Users.Get(FirstInstanceUsersKey, u).ID +} + func (s *Tester) WithAuthorizationToken(ctx context.Context, token string) context.Context { md, ok := metadata.FromOutgoingContext(ctx) if !ok { diff --git a/internal/query/user.go b/internal/query/user.go index 0edd8deb34..5209f1b08c 100644 --- a/internal/query/user.go +++ b/internal/query/user.go @@ -127,7 +127,7 @@ func (u *Users) RemoveNoPermission(ctx context.Context, permissionCheck domain.P for i := range u.Users { ctxData := authz.GetCtxData(ctx) if ctxData.UserID != u.Users[i].ID { - if err := permissionCheck(ctx, domain.PermissionUserRead, ctxData.OrgID, u.Users[i].ID); err != nil { + if err := permissionCheck(ctx, domain.PermissionUserRead, u.Users[i].ResourceOwner, u.Users[i].ID); err != nil { removableIndexes = append(removableIndexes, i) } }