fix issue 19928 (#20409)

* 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 <[email protected]>
goharbor · May 15, 2024 · 2977fec · 2977fec
1 parent 232f9ba
commit 2977fec
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 36 deletions.
diff --git 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},
 

diff --git 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
@@ -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
@@ -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
@@ -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/dao/dao_test.go b/src/pkg/project/dao/dao_test.go
@@ -21,6 +21,7 @@ import (
  "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"
@@ -341,6 +342,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, 2)
+ })
+ })
+ }
+}
+
 func (suite *DaoTestSuite) TestListRoles() {
  {
  // only projectAdmin

diff --git 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
@@ -21,6 +21,7 @@ import (
 
  "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"
@@ -57,15 +58,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)
+ if sc, ok := secCtx.(*local.SecurityContext); ok {
+ user := sc.User()
+ var err error
+ isProjectAdmin, err = p.mc.IsProjectAdmin(ctx, *user)
+ if err != nil {
+ return p.SendError(ctx, err)
+ }
  }
- is, err := p.mc.IsProjectAdmin(ctx, user.UserID)
- if err != nil {
- return p.SendError(ctx, err)
- }
- isProjectAdmin = is
  }
  if !isSystemAdmin && !isProjectAdmin {
  return p.SendError(ctx, errors.ForbiddenError(errors.New("only admins(system and project) can access permissions")))

diff --git a/src/testing/pkg/project/manager.go b/src/testing/pkg/project/manager.go