From 1e508f5a7fb99a342f6b6c1b46e6624a0e3a84f8 Mon Sep 17 00:00:00 2001 From: Kush Sharma Date: Thu, 21 Sep 2023 12:07:36 +0530 Subject: [PATCH] fix: org members should be allowed to get org details Signed-off-by: Kush Sharma --- .github/workflows/lint.yml | 2 +- .github/workflows/main.yml | 2 +- .github/workflows/release.yml | 4 +- .github/workflows/test.yml | 8 ++-- cmd/serve.go | 2 +- core/deleter/service.go | 22 +++++++++- core/organization/service.go | 2 +- core/serviceuser/service.go | 2 +- core/user/service.go | 6 ++- internal/api/v1beta1/audit_test.go | 10 ++--- internal/api/v1beta1/deleter.go | 1 + internal/api/v1beta1/mocks/cascade_deleter.go | 43 +++++++++++++++++++ internal/api/v1beta1/mocks/user_service.go | 43 ------------------- internal/api/v1beta1/user.go | 3 +- internal/bootstrap/schema/base_schema.zed | 2 +- .../bootstrap/testdata/compiled_schema.zed | 2 +- test/e2e/regression/api_test.go | 4 +- 17 files changed, 90 insertions(+), 68 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 16718c2d5..e08ccda0f 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -13,7 +13,7 @@ jobs: steps: - uses: actions/setup-go@v4 with: - go-version: '1.20' + go-version: '1.21' - uses: actions/checkout@v3 with: fetch-depth: 0 diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6f95bcba6..635606ba7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -15,7 +15,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: "1.20" + go-version: "1.21" - name: Login to DockerHub uses: docker/login-action@v1 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 3d47191f2..4db8d4657 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,7 +17,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: "1.20" + go-version: "1.21" - name: Login to DockerHub uses: docker/login-action@v1 with: @@ -41,7 +41,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: "1.20" + go-version: "1.21" - name: Login to DockerHub uses: docker/login-action@v1 with: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a80b9d3da..8aed8d73e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,7 +17,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: '1.20' + go-version: '1.21' - name: install dependencies run: go mod tidy - name: run unit tests @@ -42,7 +42,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: '1.20' + go-version: '1.21' - name: install dependencies run: go mod tidy - name: install spicedb binary @@ -65,7 +65,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: '1.20' + go-version: '1.21' - name: install dependencies run: go mod tidy - name: run regression tests @@ -81,7 +81,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: '1.20' + go-version: '1.21' - name: install dependencies run: go mod tidy - name: run integration tests diff --git a/cmd/serve.go b/cmd/serve.go index 75bf9db0c..7aefb9f3e 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -297,7 +297,7 @@ func buildAPIDependencies( invitationService := invitation.NewService(mailDialer, postgres.NewInvitationRepository(logger, dbc), organizationService, groupService, userService, relationService, policyService, cfg.App.Invite) cascadeDeleter := deleter.NewCascadeDeleter(organizationService, projectService, resourceService, - groupService, policyService, roleService, invitationService) + groupService, policyService, roleService, invitationService, userService) // we should default it with a stdout logger repository as postgres can start to bloat really fast var auditRepository audit.Repository diff --git a/core/deleter/service.go b/core/deleter/service.go index b1e12968b..d6ac7f329 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -29,6 +29,7 @@ type OrganizationService interface { Get(ctx context.Context, id string) (organization.Organization, error) DeleteModel(ctx context.Context, id string) error RemoveUsers(ctx context.Context, orgID string, userIDs []string) error + ListByUser(ctx context.Context, userID string) ([]organization.Organization, error) } type RoleService interface { @@ -56,6 +57,10 @@ type InvitationService interface { Delete(ctx context.Context, id uuid.UUID) error } +type UserService interface { + Delete(ctx context.Context, id string) error +} + type Service struct { projService ProjectService orgService OrganizationService @@ -64,12 +69,13 @@ type Service struct { policyService PolicyService roleService RoleService invitationService InvitationService + userService UserService } func NewCascadeDeleter(orgService OrganizationService, projService ProjectService, resService ResourceService, groupService GroupService, policyService PolicyService, roleService RoleService, - invitationService InvitationService) *Service { + invitationService InvitationService, userService UserService) *Service { return &Service{ projService: projService, orgService: orgService, @@ -78,6 +84,7 @@ func NewCascadeDeleter(orgService OrganizationService, projService ProjectServic policyService: policyService, roleService: roleService, invitationService: invitationService, + userService: userService, } } @@ -229,3 +236,16 @@ func (d Service) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs [ // remove user from org return d.orgService.RemoveUsers(ctx, orgID, userIDs) } + +func (d Service) DeleteUser(ctx context.Context, userID string) error { + userOrgs, err := d.orgService.ListByUser(ctx, userID) + if err != nil { + return err + } + for _, org := range userOrgs { + if err = d.RemoveUsersFromOrg(ctx, org.ID, []string{userID}); err != nil { + return fmt.Errorf("failed to delete user from org[%s]: %w", org.Name, err) + } + } + return d.userService.Delete(ctx, userID) +} diff --git a/core/organization/service.go b/core/organization/service.go index 2f4224e1d..92b09ec2e 100644 --- a/core/organization/service.go +++ b/core/organization/service.go @@ -199,7 +199,7 @@ func (s Service) AddUsers(ctx context.Context, orgID string, userIDs []string) e // RemoveUsers removes users from an organization as members // it doesn't remove user access to projects or other resources provided -// by policies +// by policies, don't call directly, use cascade deleter func (s Service) RemoveUsers(ctx context.Context, orgID string, userIDs []string) error { var err error for _, userID := range userIDs { diff --git a/core/serviceuser/service.go b/core/serviceuser/service.go index 276be90e3..e0920c4bd 100644 --- a/core/serviceuser/service.go +++ b/core/serviceuser/service.go @@ -141,7 +141,7 @@ func (s Service) Delete(ctx context.Context, id string) error { // delete all of serviceuser relationships // before deleting the serviceuser if err := s.relService.Delete(ctx, relation.Relation{ - Object: relation.Object{ + Subject: relation.Subject{ ID: id, Namespace: schema.ServiceUserPrincipal, }, diff --git a/core/user/service.go b/core/user/service.go index 488d18868..0bd53ac2a 100644 --- a/core/user/service.go +++ b/core/user/service.go @@ -108,10 +108,12 @@ func (s Service) Disable(ctx context.Context, id string) error { return s.repository.SetState(ctx, id, Disabled) } +// Delete by user uuid +// don't call this directly, use cascade deleter func (s Service) Delete(ctx context.Context, id string) error { - if err := s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{ + if err := s.relationService.Delete(ctx, relation.Relation{Subject: relation.Subject{ ID: id, - Namespace: schema.ProjectNamespace, + Namespace: schema.UserPrincipal, }}); err != nil { return err } diff --git a/internal/api/v1beta1/audit_test.go b/internal/api/v1beta1/audit_test.go index a45a9a913..80e6d5930 100644 --- a/internal/api/v1beta1/audit_test.go +++ b/internal/api/v1beta1/audit_test.go @@ -26,8 +26,8 @@ func TestHandler_ListOrganizationAuditLogs(t *testing.T) { { name: "should return list of audit logs", setup: func(as *mocks.AuditService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("*context.emptyCtx"), "org-id").Return(testOrgMap[testOrgID], nil) - as.EXPECT().List(mock.AnythingOfType("*context.emptyCtx"), audit.Filter{ + os.EXPECT().Get(mock.Anything, "org-id").Return(testOrgMap[testOrgID], nil) + as.EXPECT().List(mock.Anything, audit.Filter{ OrgID: testOrgMap[testOrgID].ID, Source: "guardian-service", Action: "project.create", @@ -84,8 +84,8 @@ func TestHandler_ListOrganizationAuditLogs(t *testing.T) { { name: "should return error when audit service returns error", setup: func(as *mocks.AuditService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("*context.emptyCtx"), "org-id").Return(testOrgMap[testOrgID], nil) - as.EXPECT().List(mock.AnythingOfType("*context.emptyCtx"), audit.Filter{ + os.EXPECT().Get(mock.Anything, "org-id").Return(testOrgMap[testOrgID], nil) + as.EXPECT().List(mock.Anything, audit.Filter{ OrgID: testOrgMap[testOrgID].ID, Source: "guardian-service", Action: "project.create", @@ -106,7 +106,7 @@ func TestHandler_ListOrganizationAuditLogs(t *testing.T) { { name: "should return error when org is disabled", setup: func(as *mocks.AuditService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("*context.emptyCtx"), "org-id").Return(organization.Organization{}, organization.ErrDisabled) + os.EXPECT().Get(mock.Anything, "org-id").Return(organization.Organization{}, organization.ErrDisabled) }, request: &frontierv1beta1.ListOrganizationAuditLogsRequest{ OrgId: "org-id", diff --git a/internal/api/v1beta1/deleter.go b/internal/api/v1beta1/deleter.go index ffa10f258..bb08eb16c 100644 --- a/internal/api/v1beta1/deleter.go +++ b/internal/api/v1beta1/deleter.go @@ -13,6 +13,7 @@ type CascadeDeleter interface { DeleteProject(ctx context.Context, id string) error DeleteOrganization(ctx context.Context, id string) error RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error + DeleteUser(ctx context.Context, userID string) error } func (h Handler) DeleteProject(ctx context.Context, request *frontierv1beta1.DeleteProjectRequest) (*frontierv1beta1.DeleteProjectResponse, error) { diff --git a/internal/api/v1beta1/mocks/cascade_deleter.go b/internal/api/v1beta1/mocks/cascade_deleter.go index 697f96395..b6d607612 100644 --- a/internal/api/v1beta1/mocks/cascade_deleter.go +++ b/internal/api/v1beta1/mocks/cascade_deleter.go @@ -107,6 +107,49 @@ func (_c *CascadeDeleter_DeleteProject_Call) RunAndReturn(run func(context.Conte return _c } +// DeleteUser provides a mock function with given fields: ctx, userID +func (_m *CascadeDeleter) DeleteUser(ctx context.Context, userID string) error { + ret := _m.Called(ctx, userID) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, userID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// CascadeDeleter_DeleteUser_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteUser' +type CascadeDeleter_DeleteUser_Call struct { + *mock.Call +} + +// DeleteUser is a helper method to define mock.On call +// - ctx context.Context +// - userID string +func (_e *CascadeDeleter_Expecter) DeleteUser(ctx interface{}, userID interface{}) *CascadeDeleter_DeleteUser_Call { + return &CascadeDeleter_DeleteUser_Call{Call: _e.mock.On("DeleteUser", ctx, userID)} +} + +func (_c *CascadeDeleter_DeleteUser_Call) Run(run func(ctx context.Context, userID string)) *CascadeDeleter_DeleteUser_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *CascadeDeleter_DeleteUser_Call) Return(_a0 error) *CascadeDeleter_DeleteUser_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *CascadeDeleter_DeleteUser_Call) RunAndReturn(run func(context.Context, string) error) *CascadeDeleter_DeleteUser_Call { + _c.Call.Return(run) + return _c +} + // RemoveUsersFromOrg provides a mock function with given fields: ctx, orgID, userIDs func (_m *CascadeDeleter) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error { ret := _m.Called(ctx, orgID, userIDs) diff --git a/internal/api/v1beta1/mocks/user_service.go b/internal/api/v1beta1/mocks/user_service.go index d91a744fb..1c57354a3 100644 --- a/internal/api/v1beta1/mocks/user_service.go +++ b/internal/api/v1beta1/mocks/user_service.go @@ -75,49 +75,6 @@ func (_c *UserService_Create_Call) RunAndReturn(run func(context.Context, user.U return _c } -// Delete provides a mock function with given fields: ctx, id -func (_m *UserService) Delete(ctx context.Context, id string) error { - ret := _m.Called(ctx, id) - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { - r0 = rf(ctx, id) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// UserService_Delete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Delete' -type UserService_Delete_Call struct { - *mock.Call -} - -// Delete is a helper method to define mock.On call -// - ctx context.Context -// - id string -func (_e *UserService_Expecter) Delete(ctx interface{}, id interface{}) *UserService_Delete_Call { - return &UserService_Delete_Call{Call: _e.mock.On("Delete", ctx, id)} -} - -func (_c *UserService_Delete_Call) Run(run func(ctx context.Context, id string)) *UserService_Delete_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string)) - }) - return _c -} - -func (_c *UserService_Delete_Call) Return(_a0 error) *UserService_Delete_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *UserService_Delete_Call) RunAndReturn(run func(context.Context, string) error) *UserService_Delete_Call { - _c.Call.Return(run) - return _c -} - // Disable provides a mock function with given fields: ctx, id func (_m *UserService) Disable(ctx context.Context, id string) error { ret := _m.Called(ctx, id) diff --git a/internal/api/v1beta1/user.go b/internal/api/v1beta1/user.go index 8a5be873c..1bab7ca33 100644 --- a/internal/api/v1beta1/user.go +++ b/internal/api/v1beta1/user.go @@ -44,7 +44,6 @@ type UserService interface { Update(ctx context.Context, toUpdate user.User) (user.User, error) Enable(ctx context.Context, id string) error Disable(ctx context.Context, id string) error - Delete(ctx context.Context, id string) error IsSudo(ctx context.Context, id string) (bool, error) } @@ -664,7 +663,7 @@ func (h Handler) DisableUser(ctx context.Context, request *frontierv1beta1.Disab func (h Handler) DeleteUser(ctx context.Context, request *frontierv1beta1.DeleteUserRequest) (*frontierv1beta1.DeleteUserResponse, error) { logger := grpczap.Extract(ctx) - if err := h.userService.Delete(ctx, request.GetId()); err != nil { + if err := h.deleterService.DeleteUser(ctx, request.GetId()); err != nil { logger.Error(err.Error()) return nil, status.Errorf(codes.Internal, err.Error()) } diff --git a/internal/bootstrap/schema/base_schema.zed b/internal/bootstrap/schema/base_schema.zed index e86836b7f..5f64344d2 100644 --- a/internal/bootstrap/schema/base_schema.zed +++ b/internal/bootstrap/schema/base_schema.zed @@ -32,7 +32,7 @@ definition app/organization { permission delete = platform->superuser + granted->app_organization_administer + granted->app_organization_delete + owner permission update = platform->superuser + granted->app_organization_administer + granted->app_organization_update + owner - permission get = platform->superuser + granted->app_organization_administer + granted->app_organization_get + owner + permission get = platform->superuser + granted->app_organization_administer + granted->app_organization_get + owner + member permission rolemanage = platform->superuser + granted->app_organization_administer + granted->app_organization_rolemanage + owner permission policymanage = platform->superuser + granted->app_organization_administer + granted->app_organization_policymanage + owner permission projectlist = platform->superuser + granted->app_organization_administer + granted->app_organization_projectlist + owner diff --git a/internal/bootstrap/testdata/compiled_schema.zed b/internal/bootstrap/testdata/compiled_schema.zed index 017b271fa..0957b7b71 100644 --- a/internal/bootstrap/testdata/compiled_schema.zed +++ b/internal/bootstrap/testdata/compiled_schema.zed @@ -31,7 +31,7 @@ definition app/organization { permission compute_receipt_get = owner + platform->superuser + granted->app_organization_administer + granted->compute_receipt_get permission compute_receipt_update = owner + platform->superuser + granted->app_organization_administer + granted->compute_receipt_update permission delete = platform->superuser + granted->app_organization_administer + granted->app_organization_delete + owner - permission get = platform->superuser + granted->app_organization_administer + granted->app_organization_get + owner + permission get = platform->superuser + granted->app_organization_administer + granted->app_organization_get + owner + member relation granted: app/rolebinding // synthetic permissions - group diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index eebdcc375..0c5b3f914 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -694,7 +694,7 @@ func (s *APIRegressionTestSuite) TestUserAPI() { PermissionFilter: schema.GetPermission, }) s.Assert().NoError(err) - s.Assert().Equal(2, len(orgUsersRespAfterRelation.GetUsers())) + s.Assert().Equal(3, len(orgUsersRespAfterRelation.GetUsers())) groupUsersResp, err := s.testBench.Client.ListGroupUsers(ctxOrgAdminAuth, &frontierv1beta1.ListGroupUsersRequest{ Id: existingGroup.Id, OrgId: existingOrg.GetOrganization().GetId(), @@ -735,7 +735,7 @@ func (s *APIRegressionTestSuite) TestUserAPI() { PermissionFilter: schema.GetPermission, }) s.Assert().NoError(err) - s.Assert().Equal(1, len(orgUsersRespAfterDeletion.GetUsers())) + s.Assert().Equal(2, len(orgUsersRespAfterDeletion.GetUsers())) // check its relations with group groupUsersRespAfterDeletion, err := s.testBench.Client.ListGroupUsers(ctxOrgAdminAuth, &frontierv1beta1.ListGroupUsersRequest{