Revert package access change from #23879 (#25707) (#25785)

Backport #25707 by @KN4CK3R

Fixes (?) #25538
Fixes https://codeberg.org/forgejo/forgejo/issues/972

Regression #23879

#23879 introduced a change which prevents read access to packages if a
user is not a member of an organization.

That PR also contained a change which disallows package access if the
team unit is configured with "no access" for packages. I don't think
this change makes sense (at the moment). It may be relevant for private
orgs. But for public or limited orgs that's useless because an
unauthorized user would have more access rights than the team member.
This PR restores the old behaviour "If a user has read access for an
owner, they can read packages".

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

models/fixtures/org_user.yml

@@ -81,3 +81,21 @@
   uid: 5
   org_id: 23
   is_public: false
+
+-
+  id: 15
+  uid: 1
+  org_id: 35
+  is_public: true
+
+-
+  id: 16
+  uid: 1
+  org_id: 36
+  is_public: true
+
+-
+  id: 17
+  uid: 5
+  org_id: 36
+  is_public: true

models/fixtures/team.yml

@@ -184,3 +184,36 @@
   num_members: 1
   includes_all_repositories: false
   can_create_org_repo: true
+
+-
+  id: 18
+  org_id: 35
+  lower_name: owners
+  name: Owners
+  authorize: 4 # owner
+  num_repos: 0
+  num_members: 1
+  includes_all_repositories: false
+  can_create_org_repo: true
+
+-
+  id: 19
+  org_id: 36
+  lower_name: owners
+  name: Owners
+  authorize: 4 # owner
+  num_repos: 0
+  num_members: 1
+  includes_all_repositories: false
+  can_create_org_repo: true
+
+-
+  id: 20
+  org_id: 36
+  lower_name: team20writepackage
+  name: team20writepackage
+  authorize: 1
+  num_repos: 0
+  num_members: 1
+  includes_all_repositories: false
+  can_create_org_repo: true

models/fixtures/team_unit.yml

@@ -273,4 +273,10 @@
   id: 46
   team_id: 17
   type: 9 # package
-  access_mode: 0
+  access_mode: 2
+
+-
+  id: 47
+  team_id: 20
+  type: 9 # package
+  access_mode: 2

models/fixtures/team_user.yml

@@ -105,3 +105,21 @@
   org_id: 23
   team_id: 17
   uid: 5
+
+-
+  id: 19
+  org_id: 35
+  team_id: 18
+  uid: 1
+
+-
+  id: 20
+  org_id: 36
+  team_id: 19
+  uid: 1
+
+-
+  id: 21
+  org_id: 36
+  team_id: 20
+  uid: 5

models/fixtures/user.yml

@@ -1258,3 +1258,77 @@
   repo_admin_change_team_access: false
   theme: ""
   keep_activity_private: false
+
+-
+  id: 35
+  lower_name: private_org35
+  name: private_org35
+  full_name: Private Org 35
+  email: private_org35@example.com
+  keep_email_private: false
+  email_notifications_preference: enabled
+  passwd: ZogKvWdyEx:password
+  passwd_hash_algo: dummy
+  must_change_password: false
+  login_source: 0
+  login_name: private_org35
+  type: 1
+  salt: ZogKvWdyEx
+  max_repo_creation: -1
+  is_active: true
+  is_admin: false
+  is_restricted: false
+  allow_git_hook: false
+  allow_import_local: false
+  allow_create_organization: true
+  prohibit_login: false
+  avatar: avatar35
+  avatar_email: private_org35@example.com
+  use_custom_avatar: false
+  num_followers: 0
+  num_following: 0
+  num_stars: 0
+  num_repos: 0
+  num_teams: 1
+  num_members: 1
+  visibility: 2
+  repo_admin_change_team_access: false
+  theme: ""
+  keep_activity_private: false
+
+-
+  id: 36
+  lower_name: limited_org36
+  name: limited_org36
+  full_name: Limited Org 36
+  email: limited_org36@example.com
+  keep_email_private: false
+  email_notifications_preference: enabled
+  passwd: ZogKvWdyEx:password
+  passwd_hash_algo: dummy
+  must_change_password: false
+  login_source: 0
+  login_name: limited_org36
+  type: 1
+  salt: ZogKvWdyEx
+  max_repo_creation: -1
+  is_active: true
+  is_admin: false
+  is_restricted: false
+  allow_git_hook: false
+  allow_import_local: false
+  allow_create_organization: true
+  prohibit_login: false
+  avatar: avatar22
+  avatar_email: limited_org36@example.com
+  use_custom_avatar: false
+  num_followers: 0
+  num_following: 0
+  num_stars: 0
+  num_repos: 0
+  num_teams: 2
+  num_members: 2
+  visibility: 1
+  repo_admin_change_team_access: false
+  theme: ""
+  keep_activity_private: false

modules/context/package.go

@@ -108,18 +108,28 @@ func determineAccessMode(ctx *Base, pkg *Package, doer *user_model.User) (perm.A
 
 		if doer != nil && !doer.IsGhost() {
 			// 1. If user is logged in, check all team packages permissions
-			teams, err := organization.GetUserOrgTeams(ctx, org.ID, doer.ID)
+			var err error
+			accessMode, err = org.GetOrgUserMaxAuthorizeLevel(doer.ID)
 			if err != nil {
 				return accessMode, err
 			}
-			for _, t := range teams {
+			// If access mode is less than write check every team for more permissions
-				perm := t.UnitAccessMode(ctx, unit.TypePackages)
+			// The minimum possible access mode is read for org members
-				if accessMode < perm {
+			if accessMode < perm.AccessModeWrite {
-					accessMode = perm
+				teams, err := organization.GetUserOrgTeams(ctx, org.ID, doer.ID)
+				if err != nil {
+					return accessMode, err
+				}
+				for _, t := range teams {
+					perm := t.UnitAccessMode(ctx, unit.TypePackages)
+					if accessMode < perm {
+						accessMode = perm
+					}
 				}
 			}
-		} else if organization.HasOrgOrUserVisible(ctx, pkg.Owner, doer) {
+		}
-			// 2. If user is non-login, check if org is visible to non-login user
+		if accessMode == perm.AccessModeNone && organization.HasOrgOrUserVisible(ctx, pkg.Owner, doer) {
+			// 2. If user is unauthorized or no org member, check if org is visible
 			accessMode = perm.AccessModeRead
 		}
 	} else {

tests/integration/api_org_test.go

@@ -170,9 +170,9 @@ func TestAPIGetAll(t *testing.T) {
 	var apiOrgList []*api.Organization
 
 	DecodeJSON(t, resp, &apiOrgList)
-	assert.Len(t, apiOrgList, 9)
+	assert.Len(t, apiOrgList, 11)
-	assert.Equal(t, "org25", apiOrgList[1].FullName)
+	assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName)
-	assert.Equal(t, "public", apiOrgList[1].Visibility)
+	assert.Equal(t, "limited", apiOrgList[1].Visibility)
 
 	// accessing without a token will return only public orgs
 	req = NewRequestf(t, "GET", "/api/v1/orgs")

tests/integration/api_packages_test.go

@@ -157,29 +157,227 @@ 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})
+	limitedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 33})
+	privateUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 31})
+	privateOrgMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23}) // user has package write access
+	limitedOrgMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 36}) // user has package write access
+	publicOrgMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 25})  // user has package read access
+	privateOrgNoMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 35})
+	limitedOrgNoMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 22})
+	publicOrgNoMember := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17})
 
-	uploadPackage := func(doer, owner *user_model.User, expectedStatus int) {
+	uploadPackage := func(doer, owner *user_model.User, filename string, expectedStatus int) {
-		url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/file.bin", owner.Name)
+		url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/%s.bin", owner.Name, filename)
 		req := NewRequestWithBody(t, "PUT", url, bytes.NewReader([]byte{1}))
-		AddBasicAuthHeader(req, doer.Name)
+		if doer != nil {
+			AddBasicAuthHeader(req, doer.Name)
+		}
 		MakeRequest(t, req, expectedStatus)
 	}
 
-	uploadPackage(user, inactive, http.StatusUnauthorized)
+	downloadPackage := func(doer, owner *user_model.User, expectedStatus int) {
-	uploadPackage(inactive, inactive, http.StatusUnauthorized)
+		url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/admin.bin", owner.Name)
-	uploadPackage(inactive, user, http.StatusUnauthorized)
+		req := NewRequest(t, "GET", url)
-	uploadPackage(admin, inactive, http.StatusCreated)
+		if doer != nil {
-	uploadPackage(admin, user, http.StatusCreated)
+			AddBasicAuthHeader(req, doer.Name)
+		}
+		MakeRequest(t, req, expectedStatus)
+	}
 
-	// team.authorize is write, but team_unit.access_mode is none
+	type Target struct {
-	// so the user can not upload packages or get package list
+		Owner          *user_model.User
-	uploadPackage(user, privatedOrg, http.StatusUnauthorized)
+		ExpectedStatus int
+	}
 
-	session := loginUser(t, user.Name)
+	t.Run("Upload", func(t *testing.T) {
-	tokenReadPackage := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadPackage)
+		defer tests.PrintCurrentTest(t)()
-	req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/packages/%s?token=%s", privatedOrg.Name, tokenReadPackage))
+
-	MakeRequest(t, req, http.StatusForbidden)
+		cases := []struct {
+			Doer     *user_model.User
+			Filename string
+			Targets  []Target
+		}{
+			{ // Admins can upload to every owner
+				Doer:     admin,
+				Filename: "admin",
+				Targets: []Target{
+					{admin, http.StatusCreated},
+					{inactive, http.StatusCreated},
+					{user, http.StatusCreated},
+					{limitedUser, http.StatusCreated},
+					{privateUser, http.StatusCreated},
+					{privateOrgMember, http.StatusCreated},
+					{limitedOrgMember, http.StatusCreated},
+					{publicOrgMember, http.StatusCreated},
+					{privateOrgNoMember, http.StatusCreated},
+					{limitedOrgNoMember, http.StatusCreated},
+					{publicOrgNoMember, http.StatusCreated},
+				},
+			},
+			{ // Without credentials no upload should be possible
+				Doer:     nil,
+				Filename: "nil",
+				Targets: []Target{
+					{admin, http.StatusUnauthorized},
+					{inactive, http.StatusUnauthorized},
+					{user, http.StatusUnauthorized},
+					{limitedUser, http.StatusUnauthorized},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusUnauthorized},
+					{limitedOrgMember, http.StatusUnauthorized},
+					{publicOrgMember, http.StatusUnauthorized},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusUnauthorized},
+					{publicOrgNoMember, http.StatusUnauthorized},
+				},
+			},
+			{ // Inactive users can't upload anywhere
+				Doer:     inactive,
+				Filename: "inactive",
+				Targets: []Target{
+					{admin, http.StatusUnauthorized},
+					{inactive, http.StatusUnauthorized},
+					{user, http.StatusUnauthorized},
+					{limitedUser, http.StatusUnauthorized},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusUnauthorized},
+					{limitedOrgMember, http.StatusUnauthorized},
+					{publicOrgMember, http.StatusUnauthorized},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusUnauthorized},
+					{publicOrgNoMember, http.StatusUnauthorized},
+				},
+			},
+			{ // Normal users can upload to self and orgs in which they are members and have package write access
+				Doer:     user,
+				Filename: "user",
+				Targets: []Target{
+					{admin, http.StatusUnauthorized},
+					{inactive, http.StatusUnauthorized},
+					{user, http.StatusCreated},
+					{limitedUser, http.StatusUnauthorized},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusCreated},
+					{limitedOrgMember, http.StatusCreated},
+					{publicOrgMember, http.StatusUnauthorized},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusUnauthorized},
+					{publicOrgNoMember, http.StatusUnauthorized},
+				},
+			},
+		}
+
+		for _, c := range cases {
+			for _, t := range c.Targets {
+				uploadPackage(c.Doer, t.Owner, c.Filename, t.ExpectedStatus)
+			}
+		}
+	})
+
+	t.Run("Download", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		cases := []struct {
+			Doer     *user_model.User
+			Filename string
+			Targets  []Target
+		}{
+			{ // Admins can access everything
+				Doer: admin,
+				Targets: []Target{
+					{admin, http.StatusOK},
+					{inactive, http.StatusOK},
+					{user, http.StatusOK},
+					{limitedUser, http.StatusOK},
+					{privateUser, http.StatusOK},
+					{privateOrgMember, http.StatusOK},
+					{limitedOrgMember, http.StatusOK},
+					{publicOrgMember, http.StatusOK},
+					{privateOrgNoMember, http.StatusOK},
+					{limitedOrgNoMember, http.StatusOK},
+					{publicOrgNoMember, http.StatusOK},
+				},
+			},
+			{ // Without credentials only public owners are accessible
+				Doer: nil,
+				Targets: []Target{
+					{admin, http.StatusOK},
+					{inactive, http.StatusOK},
+					{user, http.StatusOK},
+					{limitedUser, http.StatusUnauthorized},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusUnauthorized},
+					{limitedOrgMember, http.StatusUnauthorized},
+					{publicOrgMember, http.StatusOK},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusUnauthorized},
+					{publicOrgNoMember, http.StatusOK},
+				},
+			},
+			{ // Inactive users have no access
+				Doer: inactive,
+				Targets: []Target{
+					{admin, http.StatusUnauthorized},
+					{inactive, http.StatusUnauthorized},
+					{user, http.StatusUnauthorized},
+					{limitedUser, http.StatusUnauthorized},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusUnauthorized},
+					{limitedOrgMember, http.StatusUnauthorized},
+					{publicOrgMember, http.StatusUnauthorized},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusUnauthorized},
+					{publicOrgNoMember, http.StatusUnauthorized},
+				},
+			},
+			{ // Normal users can access self, public or limited users/orgs and private orgs in which they are members
+				Doer: user,
+				Targets: []Target{
+					{admin, http.StatusOK},
+					{inactive, http.StatusOK},
+					{user, http.StatusOK},
+					{limitedUser, http.StatusOK},
+					{privateUser, http.StatusUnauthorized},
+					{privateOrgMember, http.StatusOK},
+					{limitedOrgMember, http.StatusOK},
+					{publicOrgMember, http.StatusOK},
+					{privateOrgNoMember, http.StatusUnauthorized},
+					{limitedOrgNoMember, http.StatusOK},
+					{publicOrgNoMember, http.StatusOK},
+				},
+			},
+		}
+
+		for _, c := range cases {
+			for _, target := range c.Targets {
+				downloadPackage(c.Doer, target.Owner, target.ExpectedStatus)
+			}
+		}
+	})
+
+	t.Run("API", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		session := loginUser(t, user.Name)
+		tokenReadPackage := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadPackage)
+
+		for _, target := range []Target{
+			{admin, http.StatusOK},
+			{inactive, http.StatusOK},
+			{user, http.StatusOK},
+			{limitedUser, http.StatusOK},
+			{privateUser, http.StatusForbidden},
+			{privateOrgMember, http.StatusOK},
+			{limitedOrgMember, http.StatusOK},
+			{publicOrgMember, http.StatusOK},
+			{privateOrgNoMember, http.StatusForbidden},
+			{limitedOrgNoMember, http.StatusOK},
+			{publicOrgNoMember, http.StatusOK},
+		} {
+			req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/packages/%s?token=%s", target.Owner.Name, tokenReadPackage))
+			MakeRequest(t, req, target.ExpectedStatus)
+		}
+	})
 }
 
 func TestPackageQuota(t *testing.T) {