Refactor CheckLoginPermission() code

controllers/auth.go

@@ -59,7 +59,7 @@ func tokenToResponse(token *object.Token) *Response {
 func (c *ApiController) HandleLoggedIn(application *object.Application, user *object.User, form *form.AuthForm) (resp *Response) {
 	userId := user.GetId()
 
-	allowed, err := object.CheckAccessPermission(userId, application)
+	allowed, err := object.CheckLoginPermission(userId, application)
 	if err != nil {
 		c.ResponseError(err.Error(), nil)
 		return

object/check.go

@@ -350,7 +350,7 @@ func CheckUserPermission(requestUserId, userId string, strict bool, lang string)
 	return hasPermission, fmt.Errorf(i18n.Translate(lang, "auth:Unauthorized operation"))
 }
 
-func CheckAccessPermission(userId string, application *Application) (bool, error) {
+func CheckLoginPermission(userId string, application *Application) (bool, error) {
 	var err error
 	if userId == "built-in/admin" {
 		return true, nil
@@ -361,32 +361,40 @@ func CheckAccessPermission(userId string, application *Application) (bool, error
 		return false, err
 	}
 
-	allowed := true
+	allowCount := 0
+	denyCount := 0
 	for _, permission := range permissions {
-		if !permission.IsEnabled {
+		if !permission.IsEnabled || permission.ResourceType != "Application" || !permission.isResourceHit(application.Name) {
 			continue
 		}
 
-		isHit := false
-		for _, resource := range permission.Resources {
-			if application.Name == resource {
-				isHit = true
-				break
+		if permission.isUserHit(userId) {
+			allowCount += 1
+		}
+
+		enforcer := getPermissionEnforcer(permission)
+
+		var isAllowed bool
+		isAllowed, err = enforcer.Enforce(userId, application.Name, "Read")
+		if err != nil {
+			return false, err
+		}
+
+		if isAllowed {
+			if permission.Effect == "Allow" {
+				allowCount += 1
+			}
+		} else {
+			if permission.Effect == "Deny" {
+				denyCount += 1
+			}
 		}
 	}
 
-		if isHit {
-			containsAsterisk := ContainsAsterisk(userId, permission.Users)
-			if containsAsterisk {
-				return true, err
+	if denyCount > 0 {
+		return false, nil
 	}
-			enforcer := getPermissionEnforcer(permission)
-			if allowed, err = enforcer.Enforce(userId, application.Name, "read"); allowed {
-				return allowed, err
-			}
-		}
-	}
-	return allowed, err
+	return true, nil
 }
 
 func CheckUsername(username string, lang string) string {

object/permission.go

@@ -61,10 +61,6 @@ type PermissionRule struct {
 
 const builtInAvailableField = 5 // Casdoor built-in adapter, use V5 to filter permission, so has 5 available field
 
-func (p *Permission) GetId() string {
-	return util.GetId(p.Owner, p.Name)
-}
-
 func GetPermissionCount(owner, field, value string) (int64, error) {
 	session := GetSession(owner, -1, -1, field, value, "", "")
 	return session.Count(&Permission{})
@@ -346,20 +342,6 @@ func GetPermissionsByModel(owner string, model string) ([]*Permission, error) {
 	return permissions, nil
 }
 
-func ContainsAsterisk(userId string, users []string) bool {
-	containsAsterisk := false
-	group, _ := util.GetOwnerAndNameFromId(userId)
-	for _, user := range users {
-		permissionGroup, permissionUserName := util.GetOwnerAndNameFromId(user)
-		if permissionGroup == group && permissionUserName == "*" {
-			containsAsterisk = true
-			break
-		}
-	}
-
-	return containsAsterisk
-}
-
 func GetMaskedPermissions(permissions []*Permission) []*Permission {
 	for _, permission := range permissions {
 		permission.Users = nil
@@ -389,3 +371,27 @@ func GroupPermissionsByModelAdapter(permissions []*Permission) map[string][]stri
 
 	return m
 }
+
+func (p *Permission) GetId() string {
+	return util.GetId(p.Owner, p.Name)
+}
+
+func (p *Permission) isUserHit(name string) bool {
+	targetOrg, _ := util.GetOwnerAndNameFromId(name)
+	for _, user := range p.Users {
+		userOrg, userName := util.GetOwnerAndNameFromId(user)
+		if userOrg == targetOrg && userName == "*" {
+			return true
+		}
+	}
+	return false
+}
+
+func (p *Permission) isResourceHit(name string) bool {
+	for _, resource := range p.Resources {
+		if name == resource {
+			return true
+		}
+	}
+	return false
+}