Improve permission check of packages (#23879)

At first, we have one unified team unit permission which is called
`Team.Authorize` in DB.
But since #17811, we allowed
different units to have different permission.

The old code is only designed for the old version. So after #17811, if
org users have write permission of other units, but have no permission
of packages, they can also get write permission of packages.

Co-authored-by: delvh <[email protected]>

Author: yp05327

models/fixtures/org_user.yml

@@ -75,3 +75,9 @@
   uid: 31
   org_id: 19
   is_public: true
+
+-
+  id: 14
+  uid: 5
+  org_id: 23
+  is_public: false

models/fixtures/team.yml

@@ -172,4 +172,15 @@
   num_repos: 0
   num_members: 0
   includes_all_repositories: false
-  can_create_org_repo: true
+  can_create_org_repo: true
+
+-
+  id: 17
+  org_id: 23
+  lower_name: team14writeauth
+  name: team14WriteAuth
+  authorize: 2 # write
+  num_repos: 0
+  num_members: 1
+  includes_all_repositories: false
+  can_create_org_repo: true

models/fixtures/team_unit.yml

@@ -268,3 +268,9 @@
   team_id: 9
   type: 1 # code
   access_mode: 1
+
+-
+  id: 46
+  team_id: 17
+  type: 9 # package
+  access_mode: 0

models/fixtures/team_user.yml

@@ -99,3 +99,9 @@
   org_id: 3
   team_id: 14
   uid: 2
+
+-
+  id: 18
+  org_id: 23
+  team_id: 17
+  uid: 5

models/fixtures/user.yml

@@ -844,8 +844,8 @@
   num_following: 0
   num_stars: 0
   num_repos: 2
-  num_teams: 1
-  num_members: 0
+  num_teams: 2
+  num_members: 1
   visibility: 2
   repo_admin_change_team_access: false
   theme: ""

models/organization/org_test.go

@@ -212,25 +212,31 @@ func TestGetOrgUsersByUserID(t *testing.T) {
 
 	orgUsers, err := organization.GetOrgUsersByUserID(5, &organization.SearchOrganizationsOptions{All: true})
 	assert.NoError(t, err)
-	if assert.Len(t, orgUsers, 2) {
+	if assert.Len(t, orgUsers, 3) {
 		assert.Equal(t, organization.OrgUser{
 			ID:       orgUsers[0].ID,
-			OrgID:    6,
+			OrgID:    23,
 			UID:      5,
-			IsPublic: true,
+			IsPublic: false,
 		}, *orgUsers[0])
 		assert.Equal(t, organization.OrgUser{
 			ID:       orgUsers[1].ID,
+			OrgID:    6,
+			UID:      5,
+			IsPublic: true,
+		}, *orgUsers[1])
+		assert.Equal(t, organization.OrgUser{
+			ID:       orgUsers[2].ID,
 			OrgID:    7,
 			UID:      5,
 			IsPublic: false,
-		}, *orgUsers[1])
+		}, *orgUsers[2])
 	}
 
 	publicOrgUsers, err := organization.GetOrgUsersByUserID(5, &organization.SearchOrganizationsOptions{All: false})
 	assert.NoError(t, err)
 	assert.Len(t, publicOrgUsers, 1)
-	assert.Equal(t, *orgUsers[0], *publicOrgUsers[0])
+	assert.Equal(t, *orgUsers[1], *publicOrgUsers[0])
 
 	orgUsers, err = organization.GetOrgUsersByUserID(1, &organization.SearchOrganizationsOptions{All: true})
 	assert.NoError(t, err)

modules/context/package.go

@@ -92,33 +92,25 @@ func determineAccessMode(ctx *Context) (perm.AccessMode, error) {
 		return perm.AccessModeNone, nil
 	}
 
+	// TODO: ActionUser permission check
 	accessMode := perm.AccessModeNone
 	if ctx.Package.Owner.IsOrganization() {
 		org := organization.OrgFromUser(ctx.Package.Owner)
 
-		// 1. Get user max authorize level for the org (may be none, if user is not member of the org)
-		if ctx.Doer != nil {
-			var err error
-			accessMode, err = org.GetOrgUserMaxAuthorizeLevel(ctx.Doer.ID)
+		if ctx.Doer != nil && !ctx.Doer.IsGhost() {
+			// 1. If user is logged in, check all team packages permissions
+			teams, err := organization.GetUserOrgTeams(ctx, org.ID, ctx.Doer.ID)
 			if err != nil {
 				return accessMode, err
 			}
-			// If access mode is less than write check every team for more permissions
-			if accessMode < perm.AccessModeWrite {
-				teams, err := organization.GetUserOrgTeams(ctx, org.ID, ctx.Doer.ID)
-				if err != nil {
-					return accessMode, err
-				}
-				for _, t := range teams {
-					perm := t.UnitAccessMode(ctx, unit.TypePackages)
-					if accessMode < perm {
-						accessMode = perm
-					}
+			for _, t := range teams {
+				perm := t.UnitAccessMode(ctx, unit.TypePackages)
+				if accessMode < perm {
+					accessMode = perm
 				}
 			}
-		}
-		// 2. If authorize level is none, check if org is visible to user
-		if accessMode == perm.AccessModeNone && organization.HasOrgOrUserVisible(ctx, ctx.Package.Owner, ctx.Doer) {
+		} else if organization.HasOrgOrUserVisible(ctx, ctx.Package.Owner, ctx.Doer) {
+			// 2. If user is non-login, check if org is visible to non-login user
 			accessMode = perm.AccessModeRead
 		}
 	} else {

tests/integration/api_packages_test.go

@@ -157,6 +157,7 @@ func TestPackageAccess(t *testing.T) {
 	admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
 	inactive := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 9})
+	privatedOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23})
 
 	uploadPackage := func(doer, owner *user_model.User, expectedStatus int) {
 		url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/file.bin", owner.Name)
@@ -170,6 +171,15 @@ func TestPackageAccess(t *testing.T) {
 	uploadPackage(inactive, user, http.StatusUnauthorized)
 	uploadPackage(admin, inactive, http.StatusCreated)
 	uploadPackage(admin, user, http.StatusCreated)
+
+	// team.authorize is write, but team_unit.access_mode is none
+	// so the user can not upload packages or get package list
+	uploadPackage(user, privatedOrg, http.StatusUnauthorized)
+
+	session := loginUser(t, user.Name)
+	tokenReadPackage := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadPackage)
+	req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/packages/%s?token=%s", privatedOrg.Name, tokenReadPackage))
+	MakeRequest(t, req, http.StatusForbidden)
 }
 
 func TestPackageQuota(t *testing.T) {