From e6dd128575fe0db4770b49b4d88764d3e33100cc Mon Sep 17 00:00:00 2001 From: wang yan Date: Thu, 9 May 2024 15:01:24 +0800 Subject: [PATCH 1/6] fix issue 19928 it needs to consider the user who is in any group that has been granted with the project admin role. Signed-off-by: wang yan --- src/controller/member/controller.go | 7 ++--- src/controller/member/controller_test.go | 2 +- src/pkg/cached/project/redis/manager.go | 5 ++-- src/pkg/project/dao/dao.go | 33 +++++++++++++++++++----- src/pkg/project/manager.go | 7 ++--- src/server/v2.0/handler/permissions.go | 2 +- src/testing/pkg/project/manager.go | 23 ++++++++++------- 7 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/controller/member/controller.go b/src/controller/member/controller.go index 80212d9c52e..c53d49232d3 100644 --- a/src/controller/member/controller.go +++ b/src/controller/member/controller.go @@ -19,6 +19,7 @@ import ( "fmt" "github.com/goharbor/harbor/src/common" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/core/auth" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/q" @@ -45,7 +46,7 @@ type Controller interface { // Count get the total amount of project members Count(ctx context.Context, projectNameOrID interface{}, query *q.Query) (int, error) // IsProjectAdmin judges if the user is a project admin of any project - IsProjectAdmin(ctx context.Context, memberID int) (bool, error) + IsProjectAdmin(ctx context.Context, member commonmodels.User) (bool, error) } // Request - Project Member Request @@ -261,8 +262,8 @@ func (c *controller) Delete(ctx context.Context, projectNameOrID interface{}, me return c.mgr.Delete(ctx, p.ProjectID, memberID) } -func (c *controller) IsProjectAdmin(ctx context.Context, memberID int) (bool, error) { - members, err := c.projectMgr.ListAdminRolesOfUser(ctx, memberID) +func (c *controller) IsProjectAdmin(ctx context.Context, member commonmodels.User) (bool, error) { + members, err := c.projectMgr.ListAdminRolesOfUser(ctx, member) if err != nil { return false, err } diff --git a/src/controller/member/controller_test.go b/src/controller/member/controller_test.go index 56348166060..fe548750920 100644 --- a/src/controller/member/controller_test.go +++ b/src/controller/member/controller_test.go @@ -98,7 +98,7 @@ func (suite *MemberControllerTestSuite) TestAddProjectMemberWithUserGroup() { func (suite *MemberControllerTestSuite) TestIsProjectAdmin() { mock.OnAnything(suite.projectMgr, "ListAdminRolesOfUser").Return([]models.Member{models.Member{ID: 2, ProjectID: 2}}, nil) - ok, err := suite.controller.IsProjectAdmin(context.Background(), 2) + ok, err := suite.controller.IsProjectAdmin(context.Background(), comModels.User{UserID: 1}) suite.NoError(err) suite.True(ok) } diff --git a/src/pkg/cached/project/redis/manager.go b/src/pkg/cached/project/redis/manager.go index 2fe086b5fef..da945b817d2 100644 --- a/src/pkg/cached/project/redis/manager.go +++ b/src/pkg/cached/project/redis/manager.go @@ -18,6 +18,7 @@ import ( "context" "time" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/lib/config" "github.com/goharbor/harbor/src/lib/log" @@ -75,8 +76,8 @@ func (m *Manager) ListRoles(ctx context.Context, projectID int64, userID int, gr return m.delegator.ListRoles(ctx, projectID, userID, groupIDs...) } -func (m *Manager) ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) { - return m.delegator.ListAdminRolesOfUser(ctx, userID) +func (m *Manager) ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) { + return m.delegator.ListAdminRolesOfUser(ctx, user) } func (m *Manager) Delete(ctx context.Context, id int64) error { diff --git a/src/pkg/project/dao/dao.go b/src/pkg/project/dao/dao.go index eb88ccc477f..05d9f193bce 100644 --- a/src/pkg/project/dao/dao.go +++ b/src/pkg/project/dao/dao.go @@ -20,6 +20,7 @@ import ( "time" "github.com/goharbor/harbor/src/common" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/lib" "github.com/goharbor/harbor/src/lib/orm" "github.com/goharbor/harbor/src/lib/q" @@ -43,7 +44,7 @@ type DAO interface { // ListRoles the roles of user for the specific project ListRoles(ctx context.Context, projectID int64, userID int, groupIDs ...int) ([]int, error) // ListAdminRolesOfUser returns the roles of user for the all projects - ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) + ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) } // New returns an instance of the default DAO @@ -202,19 +203,39 @@ func (d *dao) ListRoles(ctx context.Context, projectID int64, userID int, groupI return roles, nil } -func (d *dao) ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) { +func (d *dao) ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) { o, err := orm.FromContext(ctx) if err != nil { return nil, err } - sql := `select b.* from project as a left join project_member as b on a.project_id = b.project_id where a.deleted = 'f' and b.entity_id = ? and b.entity_type = 'u' and b.role = 1;` - - var members []models.Member - _, err = o.Raw(sql, userID).QueryRows(&members) + var membersU []models.Member + sqlU := `select b.* from project as a left join project_member as b on a.project_id = b.project_id where a.deleted = 'f' and b.entity_id = ? and b.entity_type = 'u' and b.role = 1;` + _, err = o.Raw(sqlU, user.UserID).QueryRows(&membersU) if err != nil { return nil, err } + var membersG []models.Member + if len(user.GroupIDs) > 0 { + var params []interface{} + params = append(params, user.GroupIDs) + sqlG := fmt.Sprintf(`select b.* from project as a + left join project_member as b on a.project_id = b.project_id + where a.deleted = 'f' and b.entity_id in ( %s ) and b.entity_type = 'g' and b.role = 1;`, orm.ParamPlaceholderForIn(len(user.GroupIDs))) + _, err = o.Raw(sqlG, params).QueryRows(&membersG) + if err != nil { + return nil, err + } + } + + var members []models.Member + if len(membersU) > 0 { + members = append(members, membersU...) + } + if len(membersG) > 0 { + members = append(members, membersG...) + } + return members, nil } diff --git a/src/pkg/project/manager.go b/src/pkg/project/manager.go index 3a03d55d3cd..aa4d55bdb9f 100644 --- a/src/pkg/project/manager.go +++ b/src/pkg/project/manager.go @@ -19,6 +19,7 @@ import ( "regexp" "strings" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/q" @@ -47,7 +48,7 @@ type Manager interface { ListRoles(ctx context.Context, projectID int64, userID int, groupIDs ...int) ([]int, error) // ListAdminRolesOfUser returns the roles of user for the all projects - ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) + ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) } // New returns a default implementation of Manager @@ -124,6 +125,6 @@ func (m *manager) ListRoles(ctx context.Context, projectID int64, userID int, gr } // ListAdminRolesOfUser returns the roles of user for the all projects -func (m *manager) ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) { - return m.dao.ListAdminRolesOfUser(ctx, userID) +func (m *manager) ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) { + return m.dao.ListAdminRolesOfUser(ctx, user) } diff --git a/src/server/v2.0/handler/permissions.go b/src/server/v2.0/handler/permissions.go index d90ad608b1e..549ba7ab3ec 100644 --- a/src/server/v2.0/handler/permissions.go +++ b/src/server/v2.0/handler/permissions.go @@ -61,7 +61,7 @@ func (p *permissionsAPI) GetPermissions(ctx context.Context, _ permissions.GetPe if err != nil { return p.SendError(ctx, err) } - is, err := p.mc.IsProjectAdmin(ctx, user.UserID) + is, err := p.mc.IsProjectAdmin(ctx, *user) if err != nil { return p.SendError(ctx, err) } diff --git a/src/testing/pkg/project/manager.go b/src/testing/pkg/project/manager.go index 614a5e067a0..10b1c8e086e 100644 --- a/src/testing/pkg/project/manager.go +++ b/src/testing/pkg/project/manager.go @@ -5,9 +5,12 @@ package project import ( context "context" - models "github.com/goharbor/harbor/src/pkg/project/models" + commonmodels "github.com/goharbor/harbor/src/common/models" + mock "github.com/stretchr/testify/mock" + models "github.com/goharbor/harbor/src/pkg/project/models" + q "github.com/goharbor/harbor/src/lib/q" ) @@ -150,9 +153,9 @@ func (_m *Manager) List(ctx context.Context, query *q.Query) ([]*models.Project, return r0, r1 } -// ListAdminRolesOfUser provides a mock function with given fields: ctx, userID -func (_m *Manager) ListAdminRolesOfUser(ctx context.Context, userID int) ([]models.Member, error) { - ret := _m.Called(ctx, userID) +// ListAdminRolesOfUser provides a mock function with given fields: ctx, user +func (_m *Manager) ListAdminRolesOfUser(ctx context.Context, user commonmodels.User) ([]models.Member, error) { + ret := _m.Called(ctx, user) if len(ret) == 0 { panic("no return value specified for ListAdminRolesOfUser") @@ -160,19 +163,19 @@ func (_m *Manager) ListAdminRolesOfUser(ctx context.Context, userID int) ([]mode var r0 []models.Member var r1 error - if rf, ok := ret.Get(0).(func(context.Context, int) ([]models.Member, error)); ok { - return rf(ctx, userID) + if rf, ok := ret.Get(0).(func(context.Context, commonmodels.User) ([]models.Member, error)); ok { + return rf(ctx, user) } - if rf, ok := ret.Get(0).(func(context.Context, int) []models.Member); ok { - r0 = rf(ctx, userID) + if rf, ok := ret.Get(0).(func(context.Context, commonmodels.User) []models.Member); ok { + r0 = rf(ctx, user) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]models.Member) } } - if rf, ok := ret.Get(1).(func(context.Context, int) error); ok { - r1 = rf(ctx, userID) + if rf, ok := ret.Get(1).(func(context.Context, commonmodels.User) error); ok { + r1 = rf(ctx, user) } else { r1 = ret.Error(1) } From 525aa8e12786a33f4b12918cd6b27e6555156b86 Mon Sep 17 00:00:00 2001 From: wang yan Date: Sat, 11 May 2024 17:50:21 +0800 Subject: [PATCH 2/6] get user from the security Signed-off-by: wang yan --- src/pkg/project/dao/dao_test.go | 40 +++++++++++++++++++++++++- src/server/v2.0/handler/permissions.go | 17 ++++++----- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/pkg/project/dao/dao_test.go b/src/pkg/project/dao/dao_test.go index f3baf96221e..fc77ebe32d1 100644 --- a/src/pkg/project/dao/dao_test.go +++ b/src/pkg/project/dao/dao_test.go @@ -16,9 +16,11 @@ package dao import ( "fmt" + commonmodels "github.com/goharbor/harbor/src/common/models" + "github.com/stretchr/testify/suite" "testing" - "github.com/stretchr/testify/suite" + _ "github.com/stretchr/testify/suite" "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/utils" @@ -341,6 +343,42 @@ func (suite *DaoTestSuite) TestListByMember() { } } +func (suite *DaoTestSuite) TestListAdminRolesOfUser() { + { + // projectAdmin and user groups + suite.WithUser(func(userID int64, username string) { + project := &models.Project{ + Name: utils.GenerateRandomString(), + OwnerID: int(userID), + } + projectID, err := suite.dao.Create(orm.Context(), project) + suite.Nil(err) + + defer suite.dao.Delete(orm.Context(), projectID) + + suite.WithUserGroup(func(groupID int64, groupName string) { + + o, err := orm.FromContext(orm.Context()) + if err != nil { + suite.Fail("got error %v", err) + } + + var pid int64 + suite.Nil(o.Raw("INSERT INTO project_member (project_id, entity_id, role, entity_type) values (?, ?, ?, ?) RETURNING id", projectID, groupID, common.RoleProjectAdmin, "g").QueryRow(&pid)) + defer o.Raw("DELETE FROM project_member WHERE id = ?", pid) + + userTest := commonmodels.User{ + UserID: int(userID), + GroupIDs: []int{int(groupID)}, + } + roles, err := suite.dao.ListAdminRolesOfUser(orm.Context(), userTest) + suite.Nil(err) + suite.Len(roles, 1) + }) + }) + } +} + func (suite *DaoTestSuite) TestListRoles() { { // only projectAdmin diff --git a/src/server/v2.0/handler/permissions.go b/src/server/v2.0/handler/permissions.go index 549ba7ab3ec..44f331aa7b8 100644 --- a/src/server/v2.0/handler/permissions.go +++ b/src/server/v2.0/handler/permissions.go @@ -16,8 +16,8 @@ package handler import ( "context" - "github.com/go-openapi/runtime/middleware" + "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/security" @@ -57,15 +57,14 @@ func (p *permissionsAPI) GetPermissions(ctx context.Context, _ permissions.GetPe if secCtx.IsSysAdmin() { isSystemAdmin = true } else { - user, err := p.uc.GetByName(ctx, secCtx.GetUsername()) - if err != nil { - return p.SendError(ctx, err) - } - is, err := p.mc.IsProjectAdmin(ctx, *user) - if err != nil { - return p.SendError(ctx, err) + if sc, ok := secCtx.(*local.SecurityContext); ok { + user := sc.User() + is, err := p.mc.IsProjectAdmin(ctx, *user) + if err != nil { + return p.SendError(ctx, err) + } + isProjectAdmin = is } - isProjectAdmin = is } if !isSystemAdmin && !isProjectAdmin { return p.SendError(ctx, errors.ForbiddenError(errors.New("only admins(system and project) can access permissions"))) From 8e4386d663394374f1f0ae584e7594650cf8a667 Mon Sep 17 00:00:00 2001 From: wang yan Date: Sat, 11 May 2024 18:23:43 +0800 Subject: [PATCH 3/6] fix Signed-off-by: wang yan --- src/pkg/project/dao/dao_test.go | 5 ++--- src/server/v2.0/handler/permissions.go | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pkg/project/dao/dao_test.go b/src/pkg/project/dao/dao_test.go index fc77ebe32d1..43ebf3a4b27 100644 --- a/src/pkg/project/dao/dao_test.go +++ b/src/pkg/project/dao/dao_test.go @@ -16,13 +16,12 @@ package dao import ( "fmt" - commonmodels "github.com/goharbor/harbor/src/common/models" - "github.com/stretchr/testify/suite" "testing" - _ "github.com/stretchr/testify/suite" + "github.com/stretchr/testify/suite" "github.com/goharbor/harbor/src/common" + commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/orm" diff --git a/src/server/v2.0/handler/permissions.go b/src/server/v2.0/handler/permissions.go index 44f331aa7b8..90bfcde0ebf 100644 --- a/src/server/v2.0/handler/permissions.go +++ b/src/server/v2.0/handler/permissions.go @@ -16,11 +16,12 @@ package handler import ( "context" + "github.com/go-openapi/runtime/middleware" - "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/security" + "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/controller/member" "github.com/goharbor/harbor/src/controller/user" "github.com/goharbor/harbor/src/lib/errors" From a5f0835ce802b138aa3d2df524d94260d3817389 Mon Sep 17 00:00:00 2001 From: wang yan Date: Mon, 13 May 2024 14:56:55 +0800 Subject: [PATCH 4/6] resolve the review comments Signed-off-by: wang yan --- src/server/v2.0/handler/permissions.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/server/v2.0/handler/permissions.go b/src/server/v2.0/handler/permissions.go index 90bfcde0ebf..0189abf2379 100644 --- a/src/server/v2.0/handler/permissions.go +++ b/src/server/v2.0/handler/permissions.go @@ -16,7 +16,7 @@ package handler import ( "context" - + "github.com/go-openapi/runtime/middleware" "github.com/goharbor/harbor/src/common/rbac" @@ -60,11 +60,11 @@ func (p *permissionsAPI) GetPermissions(ctx context.Context, _ permissions.GetPe } else { if sc, ok := secCtx.(*local.SecurityContext); ok { user := sc.User() - is, err := p.mc.IsProjectAdmin(ctx, *user) + var err error + isProjectAdmin, err = p.mc.IsProjectAdmin(ctx, *user) if err != nil { return p.SendError(ctx, err) } - isProjectAdmin = is } } if !isSystemAdmin && !isProjectAdmin { From adf6694d64e5a1e5b67c126702d5fbd0fcfb4868 Mon Sep 17 00:00:00 2001 From: wang yan Date: Mon, 13 May 2024 16:52:02 +0800 Subject: [PATCH 5/6] fix ut Signed-off-by: wang yan --- src/pkg/project/dao/dao_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pkg/project/dao/dao_test.go b/src/pkg/project/dao/dao_test.go index 43ebf3a4b27..d054fba2b94 100644 --- a/src/pkg/project/dao/dao_test.go +++ b/src/pkg/project/dao/dao_test.go @@ -372,7 +372,7 @@ func (suite *DaoTestSuite) TestListAdminRolesOfUser() { } roles, err := suite.dao.ListAdminRolesOfUser(orm.Context(), userTest) suite.Nil(err) - suite.Len(roles, 1) + suite.Len(roles, 2) }) }) } From 3c98c172abe48b4bc757adc1d63289f06017ab8e Mon Sep 17 00:00:00 2001 From: wang yan Date: Tue, 14 May 2024 17:30:39 +0800 Subject: [PATCH 6/6] fix Signed-off-by: wang yan --- src/common/rbac/project/rbac_role.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/common/rbac/project/rbac_role.go b/src/common/rbac/project/rbac_role.go index 5ef773e9a92..961f0048698 100644 --- a/src/common/rbac/project/rbac_role.go +++ b/src/common/rbac/project/rbac_role.go @@ -125,10 +125,7 @@ var ( {Resource: rbac.ResourceMember, Action: rbac.ActionRead}, {Resource: rbac.ResourceMember, Action: rbac.ActionList}, - {Resource: rbac.ResourceMetadata, Action: rbac.ActionCreate}, {Resource: rbac.ResourceMetadata, Action: rbac.ActionRead}, - {Resource: rbac.ResourceMetadata, Action: rbac.ActionUpdate}, - {Resource: rbac.ResourceMetadata, Action: rbac.ActionDelete}, {Resource: rbac.ResourceLog, Action: rbac.ActionList},