From c163bc8317ccd3a4594f55b6a9e570e854473f84 Mon Sep 17 00:00:00 2001 From: stonezdj Date: Wed, 30 Jun 2021 17:32:22 +0800 Subject: [PATCH] Delete users under auth_mode other than db_auth The following information should cleanup before delete user: Delete project member of this user. Delete oidc_user when auth_mode is oidc_auth. Fixes #8424 It also removes the deleted user from project member and the deleted condition in the project member query for consistency Signed-off-by: stonezdj --- .../postgresql/0070_2.4.0_schema.up.sql | 2 ++ src/controller/user/controller.go | 15 +++++++++++ src/pkg/member/dao/dao.go | 16 ++++++++++-- src/pkg/member/dao/dao_test.go | 24 +++++++++++++++++ src/pkg/member/manager.go | 6 +++++ src/pkg/oidc/dao/meta.go | 12 +++++++++ src/pkg/oidc/dao/meta_test.go | 26 ++++++++++++++++--- src/pkg/oidc/metamanager.go | 6 +++++ src/server/v2.0/handler/user.go | 8 ------ src/testing/pkg/oidc/dao/meta_dao.go | 14 ++++++++++ src/testing/pkg/oidc/meta_manager.go | 14 ++++++++++ 11 files changed, 130 insertions(+), 13 deletions(-) create mode 100644 make/migrations/postgresql/0070_2.4.0_schema.up.sql diff --git a/make/migrations/postgresql/0070_2.4.0_schema.up.sql b/make/migrations/postgresql/0070_2.4.0_schema.up.sql new file mode 100644 index 000000000..8532ab881 --- /dev/null +++ b/make/migrations/postgresql/0070_2.4.0_schema.up.sql @@ -0,0 +1,2 @@ +/* cleanup deleted user project members */ +DELETE FROM project_member pm WHERE pm.entity_type = 'u' AND EXISTS (SELECT NULL FROM harbor_user u WHERE pm.entity_id = u.user_id AND u.deleted = true ) \ No newline at end of file diff --git a/src/controller/user/controller.go b/src/controller/user/controller.go index cd688520b..fa0e46874 100644 --- a/src/controller/user/controller.go +++ b/src/controller/user/controller.go @@ -17,6 +17,9 @@ package user import ( "context" "fmt" + "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/lib" + "github.com/goharbor/harbor/src/pkg/member" commonmodels "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/security" @@ -72,6 +75,7 @@ func NewController() Controller { return &controller{ mgr: user.New(), oidcMetaMgr: oidc.NewMetaMgr(), + memberMgr: member.Mgr, } } @@ -83,6 +87,7 @@ type Option struct { type controller struct { mgr user.Manager oidcMetaMgr oidc.MetaManager + memberMgr member.Manager } func (c *controller) UpdateOIDCMeta(ctx context.Context, ou *commonmodels.OIDCUser, cols ...string) error { @@ -167,6 +172,16 @@ func (c *controller) Count(ctx context.Context, query *q.Query) (int64, error) { } func (c *controller) Delete(ctx context.Context, id int) error { + // cleanup project member with the user + if err := c.memberMgr.DeleteMemberByUserID(ctx, id); err != nil { + return errors.UnknownError(err).WithMessage("delete user failed, user id: %v, cannot delete project user member, error:%v", id, err) + } + // delete oidc metadata under the user + if lib.GetAuthMode(ctx) == common.OIDCAuth { + if err := c.oidcMetaMgr.DeleteByUserID(ctx, id); err != nil { + return errors.UnknownError(err).WithMessage("delete user failed, user id: %v, cannot delete oidc user, error:%v", id, err) + } + } return c.mgr.Delete(ctx, id) } diff --git a/src/pkg/member/dao/dao.go b/src/pkg/member/dao/dao.go index 963951ef1..8c7cd3d6d 100644 --- a/src/pkg/member/dao/dao.go +++ b/src/pkg/member/dao/dao.go @@ -42,6 +42,8 @@ type DAO interface { UpdateProjectMemberRole(ctx context.Context, projectID int64, pmID int, role int) error // DeleteProjectMemberByID - Delete Project Member by ID DeleteProjectMemberByID(ctx context.Context, projectID int64, pmid int) error + // DeleteProjectMemberByUserID -- Delete project member by user id + DeleteProjectMemberByUserID(ctx context.Context, uid int) error // SearchMemberByName search members of the project by entity_name SearchMemberByName(ctx context.Context, projectID int64, entityName string) ([]*models.Member, error) // ListRoles lists the roles of user for the specific project @@ -73,7 +75,7 @@ func (d *dao) GetProjectMember(ctx context.Context, queryMember models.Member, q select pm.id as id, pm.project_id as project_id, u.user_id as entity_id, u.username as entity_name, u.creation_time, u.update_time, r.name as rolename, r.role_id as role, pm.entity_type as entity_type from harbor_user u join project_member pm on pm.project_id = ? and u.user_id = pm.entity_id - join role r on pm.role = r.role_id where u.deleted = false and pm.entity_type = 'u') as a where a.project_id = ? ` + join role r on pm.role = r.role_id where pm.entity_type = 'u') as a where a.project_id = ? ` queryParam := make([]interface{}, 1) // used ProjectID already @@ -183,6 +185,16 @@ func (d *dao) DeleteProjectMemberByID(ctx context.Context, projectID int64, pmid return nil } +func (d *dao) DeleteProjectMemberByUserID(ctx context.Context, uid int) error { + sql := "delete from project_member where entity_type = 'u' and entity_id = ? " + o, err := orm.FromContext(ctx) + if err != nil { + return err + } + _, err = o.Raw(sql, uid).Exec() + return err +} + func (d *dao) SearchMemberByName(ctx context.Context, projectID int64, entityName string) ([]*models.Member, error) { o, err := orm.FromContext(ctx) if err != nil { @@ -195,7 +207,7 @@ func (d *dao) SearchMemberByName(ctx context.Context, projectID int64, entityNam from project_member pm left join harbor_user u on pm.entity_id = u.user_id and pm.entity_type = 'u' left join role r on pm.role = r.role_id - where u.deleted = false and pm.project_id = ? and u.username like ? + where pm.project_id = ? and u.username like ? union select pm.id, pm.project_id, ug.group_name as entity_name, diff --git a/src/pkg/member/dao/dao_test.go b/src/pkg/member/dao/dao_test.go index 13b2f5739..e39f893fb 100644 --- a/src/pkg/member/dao/dao_test.go +++ b/src/pkg/member/dao/dao_test.go @@ -266,6 +266,30 @@ func (s *DaoTestSuite) TestDeleteProjectMember() { } +func (s *DaoTestSuite) TestDeleteProjectMemberByUserId() { + ctx := s.Context() + userID := 22 + var addMember = models.Member{ + ProjectID: s.projectID, + EntityID: userID, + EntityType: common.UserMember, + Role: common.RoleDeveloper, + } + pmid, err := s.dao.AddProjectMember(ctx, addMember) + s.Nil(err) + s.True(pmid > 0) + + err = s.dao.DeleteProjectMemberByUserID(ctx, userID) + s.Nil(err) + + queryMember := models.Member{ProjectID: s.projectID, EntityID: userID, EntityType: common.UserMember} + + // not exist + members, err := s.dao.GetProjectMember(ctx, queryMember, nil) + s.True(len(members) == 0) + s.Nil(err) +} + func TestDaoTestSuite(t *testing.T) { suite.Run(t, &DaoTestSuite{}) } diff --git a/src/pkg/member/manager.go b/src/pkg/member/manager.go index 51156d644..60fccddfb 100644 --- a/src/pkg/member/manager.go +++ b/src/pkg/member/manager.go @@ -41,6 +41,8 @@ type Manager interface { UpdateRole(ctx context.Context, projectID int64, pmID int, role int) error // SearchMemberByName search project member by name SearchMemberByName(ctx context.Context, projectID int64, entityName string) ([]*models.Member, error) + // DeleteMemberByUserID delete project member by user id + DeleteMemberByUserID(ctx context.Context, uid int) error // GetTotalOfProjectMembers get the total amount of project members GetTotalOfProjectMembers(ctx context.Context, projectID int64, query *q.Query, roles ...int) (int, error) // ListRoles list project roles @@ -95,6 +97,10 @@ func (m *manager) Delete(ctx context.Context, projectID int64, memberID int) err return m.dao.DeleteProjectMemberByID(ctx, projectID, memberID) } +func (m *manager) DeleteMemberByUserID(ctx context.Context, uid int) error { + return m.dao.DeleteProjectMemberByUserID(ctx, uid) +} + // NewManager ... func NewManager() Manager { return &manager{dao: dao.New()} diff --git a/src/pkg/oidc/dao/meta.go b/src/pkg/oidc/dao/meta.go index 2c280ba41..09705bf84 100644 --- a/src/pkg/oidc/dao/meta.go +++ b/src/pkg/oidc/dao/meta.go @@ -29,6 +29,8 @@ type MetaDAO interface { Create(ctx context.Context, oidcUser *models.OIDCUser) (int, error) // GetByUsername get the oidc meta record by the user's username GetByUsername(ctx context.Context, username string) (*models.OIDCUser, error) + // DeleteByUserID delete the oidc metadata by user id + DeleteByUserID(ctx context.Context, uid int) error // Update ... Update(ctx context.Context, oidcUser *models.OIDCUser, props ...string) error // List provides a way to query with flexible filter @@ -42,6 +44,16 @@ func NewMetaDao() MetaDAO { type metaDAO struct{} +func (md *metaDAO) DeleteByUserID(ctx context.Context, uid int) error { + sql := `DELETE from oidc_user where user_id = ?` + ormer, err := orm.FromContext(ctx) + if err != nil { + return err + } + _, err = ormer.Raw(sql, uid).Exec() + return err +} + func (md *metaDAO) GetByUsername(ctx context.Context, username string) (*models.OIDCUser, error) { sql := `SELECT oidc_user.id, oidc_user.user_id, oidc_user.secret, oidc_user.token, oidc_user.creation_time, oidc_user.update_time FROM oidc_user diff --git a/src/pkg/oidc/dao/meta_test.go b/src/pkg/oidc/dao/meta_test.go index d10637d96..07ba94dac 100644 --- a/src/pkg/oidc/dao/meta_test.go +++ b/src/pkg/oidc/dao/meta_test.go @@ -27,9 +27,10 @@ import ( type MetaDaoTestSuite struct { htesting.Suite - dao MetaDAO - userID int - username string + dao MetaDAO + userID int + username string + deleteUserID int } func (suite *MetaDaoTestSuite) SetupSuite() { @@ -37,8 +38,10 @@ func (suite *MetaDaoTestSuite) SetupSuite() { suite.ClearSQLs = []string{} suite.dao = NewMetaDao() suite.userID = 1234 + suite.deleteUserID = 2234 suite.username = "oidc_meta_testuser" suite.ExecSQL("INSERT INTO harbor_user (user_id, username,password,realname) VALUES(?,?,'test','test')", suite.userID, suite.username) + suite.ExecSQL("INSERT INTO harbor_user (user_id, username,password,realname) VALUES(?,?,'test','test')", suite.deleteUserID, suite.deleteUserID) ctx := orm.Context() _, err := suite.dao.Create(ctx, &models.OIDCUser{ UserID: suite.userID, @@ -48,6 +51,14 @@ func (suite *MetaDaoTestSuite) SetupSuite() { }) suite.Nil(err) suite.appendClearSQL(suite.userID) + _, err = suite.dao.Create(ctx, &models.OIDCUser{ + UserID: suite.deleteUserID, + SubIss: `ca4bb144-4b5c-4d1b-9469-69cb3768af9fhttps://sso.andrea.muellerpublic.de/auth/realms/harbor`, + Secret: `7uBP9yqtdnVAhoA243GSv8nOXBWygqzaaEdq9Kqla+q4hOaBZmEMH9vUJi4Yjbh3`, + Token: `xxxx`, + }) + suite.Nil(err) + suite.appendClearSQL(suite.deleteUserID) } func (suite *MetaDaoTestSuite) TestList() { @@ -82,6 +93,15 @@ func (suite *MetaDaoTestSuite) TestUpdate() { suite.Equal("newsecret", l[0].Secret) } +func (suite *MetaDaoTestSuite) TestDeleteByUserId() { + ctx := orm.Context() + err := suite.dao.DeleteByUserID(ctx, suite.deleteUserID) + suite.Nil(err) + l, err := suite.dao.List(ctx, q.New(q.KeyWords{"user_id": suite.deleteUserID})) + suite.Nil(err) + suite.True(len(l) == 0) +} + func (suite *MetaDaoTestSuite) appendClearSQL(uid int) { suite.ClearSQLs = append(suite.ClearSQLs, fmt.Sprintf("DELETE FROM oidc_user WHERE user_id = %d", uid)) suite.ClearSQLs = append(suite.ClearSQLs, fmt.Sprintf("DELETE FROM harbor_user WHERE user_id = %d", uid)) diff --git a/src/pkg/oidc/metamanager.go b/src/pkg/oidc/metamanager.go index 2109484d6..c20b7806c 100644 --- a/src/pkg/oidc/metamanager.go +++ b/src/pkg/oidc/metamanager.go @@ -31,6 +31,8 @@ type MetaManager interface { Create(ctx context.Context, oidcUser *models.OIDCUser) (int, error) // GetByUserID gets the oidc meta record by user's ID GetByUserID(ctx context.Context, uid int) (*models.OIDCUser, error) + // DeleteByUserID delete by user id + DeleteByUserID(ctx context.Context, uid int) error // GetBySubIss gets the oidc meta record by the subject and issuer GetBySubIss(ctx context.Context, sub, iss string) (*models.OIDCUser, error) // SetCliSecretByUserID updates the cli secret of a user based on the user ID @@ -43,6 +45,10 @@ type metaManager struct { dao dao.MetaDAO } +func (m *metaManager) DeleteByUserID(ctx context.Context, uid int) error { + return m.dao.DeleteByUserID(ctx, uid) +} + func (m *metaManager) Update(ctx context.Context, oidcUser *models.OIDCUser, cols ...string) error { return m.dao.Update(ctx, oidcUser, cols...) } diff --git a/src/server/v2.0/handler/user.go b/src/server/v2.0/handler/user.go index c5a3d8d93..d495557db 100644 --- a/src/server/v2.0/handler/user.go +++ b/src/server/v2.0/handler/user.go @@ -394,14 +394,6 @@ func (u *usersAPI) requireDeletable(ctx context.Context, id int) error { if !ok || !sctx.IsAuthenticated() { return errors.UnauthorizedError(nil) } - a, err := u.getAuth(ctx) - if err != nil { - log.G(ctx).Errorf("Failed to get authmode, error: %v", err) - return err - } - if a != common.DBAuth { - return errors.ForbiddenError(nil).WithMessage("Deleting user is not allowed under auth mode: %s", a) - } if !sctx.Can(ctx, rbac.ActionDelete, userResource) { return errors.ForbiddenError(nil).WithMessage("Not authorized to delete users") } diff --git a/src/testing/pkg/oidc/dao/meta_dao.go b/src/testing/pkg/oidc/dao/meta_dao.go index 9bfd064ec..ab8f3efa2 100644 --- a/src/testing/pkg/oidc/dao/meta_dao.go +++ b/src/testing/pkg/oidc/dao/meta_dao.go @@ -38,6 +38,20 @@ func (_m *MetaDAO) Create(ctx context.Context, oidcUser *models.OIDCUser) (int, return r0, r1 } +// DeleteByUserID provides a mock function with given fields: ctx, uid +func (_m *MetaDAO) DeleteByUserID(ctx context.Context, uid int) error { + ret := _m.Called(ctx, uid) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int) error); ok { + r0 = rf(ctx, uid) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // GetByUsername provides a mock function with given fields: ctx, username func (_m *MetaDAO) GetByUsername(ctx context.Context, username string) (*models.OIDCUser, error) { ret := _m.Called(ctx, username) diff --git a/src/testing/pkg/oidc/meta_manager.go b/src/testing/pkg/oidc/meta_manager.go index bbd8cfcea..b8dbf6188 100644 --- a/src/testing/pkg/oidc/meta_manager.go +++ b/src/testing/pkg/oidc/meta_manager.go @@ -35,6 +35,20 @@ func (_m *MetaManager) Create(ctx context.Context, oidcUser *models.OIDCUser) (i return r0, r1 } +// DeleteByUserID provides a mock function with given fields: ctx, uid +func (_m *MetaManager) DeleteByUserID(ctx context.Context, uid int) error { + ret := _m.Called(ctx, uid) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int) error); ok { + r0 = rf(ctx, uid) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // GetBySubIss provides a mock function with given fields: ctx, sub, iss func (_m *MetaManager) GetBySubIss(ctx context.Context, sub string, iss string) (*models.OIDCUser, error) { ret := _m.Called(ctx, sub, iss)