]> source.dussan.org Git - gitea.git/commitdiff
Revert package access change from #23879 (#25707)
authorKN4CK3R <admin@oldschoolhack.me>
Sun, 9 Jul 2023 13:00:07 +0000 (15:00 +0200)
committerGitHub <noreply@github.com>
Sun, 9 Jul 2023 13:00:07 +0000 (13:00 +0000)
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: Giteabot <teabot@gitea.io>
models/fixtures/org_user.yml
models/fixtures/team.yml
models/fixtures/team_unit.yml
models/fixtures/team_user.yml
models/fixtures/user.yml
modules/context/package.go
tests/integration/api_org_test.go
tests/integration/api_packages_test.go

index d08f69579913ee9904720b18b68f94e289e07270..8d58169a32f179ce30f897b2fcbff382ae82e070 100644 (file)
   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
index aa3b36e644399aac48eee8fc18130c87d6f0198e..65326eedbf47634e621177544831768ada01c1fc 100644 (file)
   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
index 5257d2c3856d392ac8a94a4934e9e8fff5d8f62e..5d2ba2fb6cbd77a1585a701f7c9c18d22091edb3 100644 (file)
   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
index b95f76c72376b7e271791ce9924a0f7391273cf9..feace5f2a531d04ce1c986d96fc5aaf97e26cf3a 100644 (file)
   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
index eba33a7c363695df779a23e4f6c8215f119dd2b8..26bb7a9f4ba891fa834a184da6b6c94215cf57be 100644 (file)
   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
index 8e80fa66ec43d9a1512958f63e5acc1b5fb888c1..be50e0a991ee2034b0fe5174f0e39bc6e71ad7f4 100644 (file)
@@ -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 {
-                               perm := t.UnitAccessMode(ctx, unit.TypePackages)
-                               if accessMode < perm {
-                                       accessMode = perm
+                       // If access mode is less than write check every team for more permissions
+                       // The minimum possible access mode is read for org members
+                       if accessMode < perm.AccessModeWrite {
+                               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 {
index edbf576b9ecf39a205dfd4b96c9ef7adf231f1a2..83a101716b099df63f8565a283852912b8e0bd2c 100644 (file)
@@ -170,9 +170,9 @@ func TestAPIGetAll(t *testing.T) {
        var apiOrgList []*api.Organization
 
        DecodeJSON(t, resp, &apiOrgList)
-       assert.Len(t, apiOrgList, 9)
-       assert.Equal(t, "org25", apiOrgList[1].FullName)
-       assert.Equal(t, "public", apiOrgList[1].Visibility)
+       assert.Len(t, apiOrgList, 11)
+       assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName)
+       assert.Equal(t, "limited", apiOrgList[1].Visibility)
 
        // accessing without a token will return only public orgs
        req = NewRequestf(t, "GET", "/api/v1/orgs")
index 84733f683b1c97fcd753dba53db0e4d54043c4ac..cd981e9c73a3125782103c99d249f29b9c5c81fb 100644 (file)
@@ -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})
-
-       uploadPackage := func(doer, owner *user_model.User, expectedStatus int) {
-               url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/file.bin", owner.Name)
+       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, filename string, expectedStatus int) {
+               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)
-       uploadPackage(inactive, inactive, http.StatusUnauthorized)
-       uploadPackage(inactive, user, http.StatusUnauthorized)
-       uploadPackage(admin, inactive, http.StatusCreated)
-       uploadPackage(admin, user, http.StatusCreated)
+       downloadPackage := func(doer, owner *user_model.User, expectedStatus int) {
+               url := fmt.Sprintf("/api/packages/%s/generic/test-package/1.0/admin.bin", owner.Name)
+               req := NewRequest(t, "GET", url)
+               if doer != nil {
+                       AddBasicAuthHeader(req, doer.Name)
+               }
+               MakeRequest(t, req, expectedStatus)
+       }
 
-       // 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)
+       type Target struct {
+               Owner          *user_model.User
+               ExpectedStatus int
+       }
 
-       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)
+       t.Run("Upload", func(t *testing.T) {
+               defer tests.PrintCurrentTest(t)()
+
+               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) {