feat: deprecate the user group relation table (#1990)

* fix: deprecate the user group relation table

* fix: clean code

* fix: fix trigger

* Update group.go

---------

Co-authored-by: hsluoyz <hsluoyz@qq.com>

controllers/user.go

@@ -556,8 +556,8 @@ func (c *ApiController) AddUserkeys() {
 func (c *ApiController) RemoveUserFromGroup() {
 	owner := c.Ctx.Request.Form.Get("owner")
 	name := c.Ctx.Request.Form.Get("name")
-	groupId := c.Ctx.Request.Form.Get("groupId")
+	groupName := c.Ctx.Request.Form.Get("groupName")
 
-	c.Data["json"] = wrapActionResponse(object.RemoveUserFromGroup(owner, name, groupId))
+	c.Data["json"] = wrapActionResponse(object.RemoveUserFromGroup(owner, name, groupName))
 	c.ServeJSON()
 }

object/adapter.go

@@ -145,11 +145,6 @@ func (a *Adapter) createTable() {
 		panic(err)
 	}
 
-	err = a.Engine.Sync2(new(UserGroupRelation))
-	if err != nil {
-		panic(err)
-	}
-
 	err = a.Engine.Sync2(new(Role))
 	if err != nil {
 		panic(err)

object/group.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 
 	"github.com/casdoor/casdoor/util"
+	"github.com/xorm-io/builder"
 	"github.com/xorm-io/core"
 )
 
@@ -28,13 +29,13 @@ type Group struct {
 	CreatedTime string `xorm:"varchar(100)" json:"createdTime"`
 	UpdatedTime string `xorm:"varchar(100)" json:"updatedTime"`
 
-	DisplayName  string    `xorm:"varchar(100)" json:"displayName"`
-	Manager      string    `xorm:"varchar(100)" json:"manager"`
-	ContactEmail string    `xorm:"varchar(100)" json:"contactEmail"`
-	Type         string    `xorm:"varchar(100)" json:"type"`
-	ParentId     string    `xorm:"varchar(100)" json:"parentId"`
-	IsTopGroup   bool      `xorm:"bool" json:"isTopGroup"`
-	Users        *[]string `xorm:"-" json:"users"`
+	DisplayName  string  `xorm:"varchar(100)" json:"displayName"`
+	Manager      string  `xorm:"varchar(100)" json:"manager"`
+	ContactEmail string  `xorm:"varchar(100)" json:"contactEmail"`
+	Type         string  `xorm:"varchar(100)" json:"type"`
+	ParentId     string  `xorm:"varchar(100)" json:"parentId"`
+	IsTopGroup   bool    `xorm:"bool" json:"isTopGroup"`
+	Users        []*User `xorm:"-" json:"users"`
 
 	Title    string   `json:"title,omitempty"`
 	Key      string   `json:"key,omitempty"`
@@ -94,24 +95,6 @@ func getGroup(owner string, name string) (*Group, error) {
 	}
 }
 
-func getGroupByName(name string) (*Group, error) {
-	if name == "" {
-		return nil, nil
-	}
-
-	group := Group{Name: name}
-	existed, err := adapter.Engine.Get(&group)
-	if err != nil {
-		return nil, err
-	}
-
-	if existed {
-		return &group, nil
-	} else {
-		return nil, nil
-	}
-}
-
 func GetGroup(id string) (*Group, error) {
 	owner, name := util.GetOwnerAndNameFromId(id)
 	return getGroup(owner, name)
@@ -124,7 +107,13 @@ func UpdateGroup(id string, group *Group) (bool, error) {
 		return false, err
 	}
 
-	group.UpdatedTime = util.GetCurrentTime()
+	if name != group.Name {
+		err := GroupChangeTrigger(name, group.Name)
+		if err != nil {
+			return false, err
+		}
+	}
+
 	affected, err := adapter.Engine.ID(core.PK{owner, name}).AllCols().Update(group)
 	if err != nil {
 		return false, err
@@ -165,31 +154,14 @@ func DeleteGroup(group *Group) (bool, error) {
 		return false, errors.New("group has children group")
 	}
 
-	if count, err := GetGroupUserCount(group.GetId(), "", ""); err != nil {
+	if count, err := GetGroupUserCount(group.Name, "", ""); err != nil {
 		return false, err
 	} else if count > 0 {
 		return false, errors.New("group has users")
 	}
 
-	session := adapter.Engine.NewSession()
-	defer session.Close()
-
-	if err := session.Begin(); err != nil {
-		return false, err
-	}
-
-	if _, err := session.Delete(&UserGroupRelation{GroupName: group.Name}); err != nil {
-		session.Rollback()
-		return false, err
-	}
-
-	affected, err := session.ID(core.PK{group.Owner, group.Name}).Delete(&Group{})
+	affected, err := adapter.Engine.ID(core.PK{group.Owner, group.Name}).Delete(&Group{})
 	if err != nil {
-		session.Rollback()
-		return false, err
-	}
-
-	if err := session.Commit(); err != nil {
 		return false, err
 	}
 
@@ -220,3 +192,113 @@ func ConvertToTreeData(groups []*Group, parentId string) []*Group {
 	}
 	return treeData
 }
+
+func RemoveUserFromGroup(owner, name, groupName string) (bool, error) {
+	user, err := getUser(owner, name)
+	if err != nil {
+		return false, err
+	}
+	if user == nil {
+		return false, errors.New("user not exist")
+	}
+
+	user.Groups = util.DeleteVal(user.Groups, groupName)
+	affected, err := updateUser(user.GetId(), user, []string{"groups"})
+	if err != nil {
+		return false, err
+	}
+	return affected != 0, err
+}
+
+func GetGroupUserCount(groupName string, field, value string) (int64, error) {
+	if field == "" && value == "" {
+		return adapter.Engine.Where(builder.Like{"`groups`", groupName}).
+			Count(&User{})
+	} else {
+		return adapter.Engine.Table("user").
+			Where(builder.Like{"`groups`", groupName}).
+			And(fmt.Sprintf("user.%s LIKE ?", util.CamelToSnakeCase(field)), "%"+value+"%").
+			Count()
+	}
+}
+
+func GetPaginationGroupUsers(groupName string, offset, limit int, field, value, sortField, sortOrder string) ([]*User, error) {
+	users := []*User{}
+	session := adapter.Engine.Table("user").
+		Where(builder.Like{"`groups`", groupName})
+
+	if offset != -1 && limit != -1 {
+		session.Limit(limit, offset)
+	}
+
+	if field != "" && value != "" {
+		session = session.And(fmt.Sprintf("user.%s LIKE ?", util.CamelToSnakeCase(field)), "%"+value+"%")
+	}
+
+	if sortField == "" || sortOrder == "" {
+		sortField = "created_time"
+	}
+	if sortOrder == "ascend" {
+		session = session.Asc(fmt.Sprintf("user.%s", util.SnakeString(sortField)))
+	} else {
+		session = session.Desc(fmt.Sprintf("user.%s", util.SnakeString(sortField)))
+	}
+
+	err := session.Find(&users)
+	if err != nil {
+		return nil, err
+	}
+
+	return users, nil
+}
+
+func GetGroupUsers(groupName string) ([]*User, error) {
+	users := []*User{}
+	err := adapter.Engine.Table("user").
+		Where(builder.Like{"`groups`", groupName}).
+		Find(&users)
+	if err != nil {
+		return nil, err
+	}
+
+	return users, nil
+}
+
+func GroupChangeTrigger(oldName, newName string) error {
+	session := adapter.Engine.NewSession()
+	defer session.Close()
+	err := session.Begin()
+	if err != nil {
+		return err
+	}
+
+	users := []*User{}
+	err = session.Where(builder.Like{"`groups`", oldName}).Find(&users)
+	if err != nil {
+		return err
+	}
+
+	for _, user := range users {
+		user.Groups = util.ReplaceVal(user.Groups, oldName, newName)
+		_, err := updateUser(user.GetId(), user, []string{"groups"})
+		if err != nil {
+			return err
+		}
+	}
+
+	groups := []*Group{}
+	err = session.Where("parent_id = ?", oldName).Find(&groups)
+	for _, group := range groups {
+		group.ParentId = newName
+		_, err := session.ID(core.PK{group.Owner, group.Name}).Cols("parent_id").Update(group)
+		if err != nil {
+			return err
+		}
+	}
+
+	err = session.Commit()
+	if err != nil {
+		return err
+	}
+	return nil
+}

object/token_jwt.go

@@ -216,6 +216,9 @@ func refineUser(user *User) *User {
 	if user.Permissions == nil {
 		user.Permissions = []*Permission{}
 	}
+	if user.Groups == nil {
+		user.Groups = []string{}
+	}
 
 	return user
 }

object/user.go

@@ -220,11 +220,11 @@ func GetPaginationGlobalUsers(offset, limit int, field, value, sortField, sortOr
 	return users, nil
 }
 
-func GetUserCount(owner, field, value string, groupId string) (int64, error) {
+func GetUserCount(owner, field, value string, groupName string) (int64, error) {
 	session := GetSession(owner, -1, -1, field, value, "", "")
 
-	if groupId != "" {
-		return GetGroupUserCount(groupId, field, value)
+	if groupName != "" {
+		return GetGroupUserCount(groupName, field, value)
 	}
 
 	return session.Count(&User{})
@@ -264,11 +264,11 @@ func GetSortedUsers(owner string, sorter string, limit int) ([]*User, error) {
 	return users, nil
 }
 
-func GetPaginationUsers(owner string, offset, limit int, field, value, sortField, sortOrder string, groupId string) ([]*User, error) {
+func GetPaginationUsers(owner string, offset, limit int, field, value, sortField, sortOrder string, groupName string) ([]*User, error) {
 	users := []*User{}
 
-	if groupId != "" {
-		return GetPaginationGroupUsers(groupId, offset, limit, field, value, sortField, sortOrder)
+	if groupName != "" {
+		return GetPaginationGroupUsers(groupName, offset, limit, field, value, sortField, sortOrder)
 	}
 
 	session := GetSessionForUser(owner, offset, limit, field, value, sortField, sortOrder)
@@ -483,17 +483,13 @@ func UpdateUser(id string, user *User, columns []string, isAdmin bool) (bool, er
 	if name != user.Name {
 		err := userChangeTrigger(name, user.Name)
 		if err != nil {
-			return false, nil
+			return false, err
 		}
 	}
 
 	if user.Password == "***" {
 		user.Password = oldUser.Password
 	}
-	err = user.UpdateUserHash()
-	if err != nil {
-		panic(err)
-	}
 
 	if user.Avatar != oldUser.Avatar && user.Avatar != "" && user.PermanentAvatar != "*" {
 		user.PermanentAvatar, err = getPermanentAvatarUrl(user.Owner, user.Name, user.Avatar, false)
@@ -521,7 +517,7 @@ func UpdateUser(id string, user *User, columns []string, isAdmin bool) (bool, er
 		columns = append(columns, "name", "email", "phone", "country_code")
 	}
 
-	affected, err := updateUser(oldUser, user, columns)
+	affected, err := updateUser(id, user, columns)
 	if err != nil {
 		return false, err
 	}
@@ -529,32 +525,17 @@ func UpdateUser(id string, user *User, columns []string, isAdmin bool) (bool, er
 	return affected != 0, nil
 }
 
-func updateUser(oldUser, user *User, columns []string) (int64, error) {
-	session := adapter.Engine.NewSession()
-	defer session.Close()
-
-	session.Begin()
-
-	if util.ContainsString(columns, "groups") {
-		affected, err := updateUserGroupRelation(session, user)
-		if err != nil {
-			session.Rollback()
-			return affected, err
-		}
-	}
-
-	affected, err := session.ID(core.PK{oldUser.Owner, oldUser.Name}).Cols(columns...).Update(user)
+func updateUser(id string, user *User, columns []string) (int64, error) {
+	owner, name := util.GetOwnerAndNameFromIdNoCheck(id)
+	err := user.UpdateUserHash()
 	if err != nil {
-		session.Rollback()
-		return affected, err
-	}
-
-	err = session.Commit()
-	if err != nil {
-		session.Rollback()
 		return 0, err
 	}
 
+	affected, err := adapter.Engine.ID(core.PK{owner, name}).Cols(columns...).Update(user)
+	if err != nil {
+		return 0, err
+	}
 	return affected, nil
 }
 
@@ -725,11 +706,6 @@ func DeleteUser(user *User) (bool, error) {
 		return false, err
 	}
 
-	affected, err = DeleteRelationByUserId(user.Id)
-	if err != nil {
-		return false, err
-	}
-
 	return affected != 0, nil
 }
 
@@ -745,7 +721,7 @@ func GetUserInfo(user *User, scope string, aud string, host string) *Userinfo {
 		resp.Name = user.Name
 		resp.DisplayName = user.DisplayName
 		resp.Avatar = user.Avatar
-		resp.Groups = []string{user.Owner}
+		resp.Groups = user.Groups
 	}
 	if strings.Contains(scope, "email") {
 		resp.Email = user.Email
@@ -781,6 +757,10 @@ func ExtendUserWithRolesAndPermissions(user *User) (err error) {
 		return err
 	}
 
+	if user.Groups == nil {
+		user.Groups = []string{}
+	}
+
 	return
 }
 

object/user_avatar.go

@@ -34,7 +34,7 @@ func downloadImage(client *http.Client, url string) (*bytes.Buffer, string, erro
 	resp, err := client.Do(req)
 	if err != nil {
 		fmt.Printf("downloadImage() error for url [%s]: %s\n", url, err.Error())
-		if strings.Contains(err.Error(), "EOF") {
+		if strings.Contains(err.Error(), "EOF") || strings.Contains(err.Error(), "no such host") {
 			return nil, "", nil
 		} else {
 			return nil, "", err

object/user_group.go

@@ -1,157 +0,0 @@
-package object
-
-import (
-	"errors"
-	"fmt"
-
-	"github.com/casdoor/casdoor/util"
-	"github.com/xorm-io/core"
-	"github.com/xorm-io/xorm"
-)
-
-type UserGroupRelation struct {
-	UserId    string `xorm:"varchar(100) notnull pk" json:"userId"`
-	GroupName string `xorm:"varchar(100) notnull pk" json:"groupName"`
-
-	CreatedTime string `xorm:"created" json:"createdTime"`
-	UpdatedTime string `xorm:"updated" json:"updatedTime"`
-}
-
-func updateUserGroupRelation(session *xorm.Session, user *User) (int64, error) {
-	physicalGroupCount, err := session.In("name", user.Groups).Count(Group{Type: "Physical"})
-	if err != nil {
-		return 0, err
-	}
-	if physicalGroupCount > 1 {
-		return 0, errors.New("user can only be in one physical group")
-	}
-
-	groups := []*Group{}
-	err = session.In("name", user.Groups).Find(&groups)
-	if err != nil {
-		return 0, err
-	}
-	if len(groups) != len(user.Groups) {
-		return 0, errors.New("group not found")
-	}
-
-	_, err = session.Delete(&UserGroupRelation{UserId: user.Id})
-	if err != nil {
-		return 0, err
-	}
-
-	relations := []*UserGroupRelation{}
-	for _, group := range groups {
-		relations = append(relations, &UserGroupRelation{UserId: user.Id, GroupName: group.Name})
-	}
-	if len(relations) == 0 {
-		return 1, nil
-	}
-	_, err = session.Insert(relations)
-	if err != nil {
-		return 0, err
-	}
-
-	return 1, nil
-}
-
-func RemoveUserFromGroup(owner, name, groupId string) (bool, error) {
-	user, err := getUser(owner, name)
-	if err != nil {
-		return false, err
-	}
-
-	groups := []string{}
-	for _, group := range user.Groups {
-		if group != groupId {
-			groups = append(groups, group)
-		}
-	}
-	user.Groups = groups
-
-	_, err = UpdateUser(util.GetId(owner, name), user, []string{"groups"}, false)
-	if err != nil {
-		return false, err
-	}
-	return true, nil
-}
-
-func DeleteUserGroupRelation(userId, groupId string) (int64, error) {
-	affected, err := adapter.Engine.ID(core.PK{userId, groupId}).Delete(&UserGroupRelation{})
-	return affected, err
-}
-
-func DeleteRelationByUserId(id string) (int64, error) {
-	affected, err := adapter.Engine.Delete(&UserGroupRelation{UserId: id})
-	return affected, err
-}
-
-func GetGroupUserCount(groupName string, field, value string) (int64, error) {
-	group, err := getGroupByName(groupName)
-	if group == nil || err != nil {
-		return 0, err
-	}
-
-	if field == "" && value == "" {
-		return adapter.Engine.Count(UserGroupRelation{GroupName: group.Name})
-	} else {
-		return adapter.Engine.Table("user").
-			Join("INNER", []string{"user_group_relation", "r"}, "user.id = r.user_id").
-			Where("r.group_name = ?", group.Name).
-			And(fmt.Sprintf("user.%s LIKE ?", util.CamelToSnakeCase(field)), "%"+value+"%").
-			Count()
-	}
-}
-
-func GetPaginationGroupUsers(groupName string, offset, limit int, field, value, sortField, sortOrder string) ([]*User, error) {
-	group, err := getGroupByName(groupName)
-	if group == nil || err != nil {
-		return nil, err
-	}
-
-	users := []*User{}
-	session := adapter.Engine.Table("user").
-		Join("INNER", []string{"user_group_relation", "r"}, "user.id = r.user_id").
-		Where("r.group_name = ?", group.Name)
-
-	if offset != -1 && limit != -1 {
-		session.Limit(limit, offset)
-	}
-
-	if field != "" && value != "" {
-		session = session.And(fmt.Sprintf("user.%s LIKE ?", util.CamelToSnakeCase(field)), "%"+value+"%")
-	}
-
-	if sortField == "" || sortOrder == "" {
-		sortField = "created_time"
-	}
-	if sortOrder == "ascend" {
-		session = session.Asc(fmt.Sprintf("user.%s", util.SnakeString(sortField)))
-	} else {
-		session = session.Desc(fmt.Sprintf("user.%s", util.SnakeString(sortField)))
-	}
-
-	err = session.Find(&users)
-	if err != nil {
-		return nil, err
-	}
-
-	return users, nil
-}
-
-func GetGroupUsers(groupName string) ([]*User, error) {
-	group, err := getGroupByName(groupName)
-	if group == nil || err != nil {
-		return []*User{}, err
-	}
-
-	users := []*User{}
-	err = adapter.Engine.Table("user").
-		Join("INNER", []string{"user_group_relation", "r"}, "user.id = r.user_id").
-		Where("r.group_name = ?", group.Name).Find(&users)
-	if err != nil {
-		return nil, err
-	}
-
-	return users, nil
-}

util/slice.go

@@ -26,6 +26,18 @@ func DeleteVal(values []string, val string) []string {
 	return newValues
 }
 
+func ReplaceVal(values []string, oldVal string, newVal string) []string {
+	newValues := []string{}
+	for _, v := range values {
+		if v == oldVal {
+			newValues = append(newValues, newVal)
+		} else {
+			newValues = append(newValues, v)
+		}
+	}
+	return newValues
+}
+
 func ContainsString(values []string, val string) bool {
 	sort.Strings(values)
 	return sort.SearchStrings(values, val) != len(values)

web/src/GroupEdit.js

@@ -131,7 +131,7 @@ class GroupEditPage extends React.Component {
             {Setting.getLabel(i18next.t("general:Name"), i18next.t("general:Name - Tooltip"))} :
           </Col>
           <Col span={22} >
-            <Input disabled={true} value={this.state.group.name} onChange={e => {
+            <Input value={this.state.group.name} onChange={e => {
               this.updateGroupField("name", e.target.value);
             }} />
           </Col>