From 372b622c2b76d0fcc1fd5623f71bd48b086a03a3 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 9 Jul 2023 17:00:42 -0400 Subject: [PATCH] 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 --- models/fixtures/org_user.yml | 18 ++ models/fixtures/team.yml | 33 ++++ models/fixtures/team_unit.yml | 8 +- models/fixtures/team_user.yml | 18 ++ models/fixtures/user.yml | 74 ++++++++ modules/context/package.go | 24 ++- tests/integration/api_org_test.go | 6 +- tests/integration/api_packages_test.go | 232 +++++++++++++++++++++++-- 8 files changed, 385 insertions(+), 28 deletions(-) diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index d08f69579913..8d58169a32f1 100644 --- a/models/fixtures/org_user.yml +++ b/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 diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index aa3b36e64439..65326eedbf47 100644 --- a/models/fixtures/team.yml +++ b/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 diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index 5257d2c3856d..5d2ba2fb6cbd 100644 --- a/models/fixtures/team_unit.yml +++ b/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 diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index b95f76c72376..feace5f2a531 100644 --- a/models/fixtures/team_user.yml +++ b/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 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index eba33a7c3636..26bb7a9f4ba8 100644 --- a/models/fixtures/user.yml +++ b/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 diff --git a/modules/context/package.go b/modules/context/package.go index 8e80fa66ec43..be50e0a991ee 100644 --- a/modules/context/package.go +++ b/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 { - 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 { diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index edbf576b9ecf..83a101716b09 100644 --- a/tests/integration/api_org_test.go +++ b/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.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") diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index 84733f683b1c..cd981e9c73a3 100644 --- a/tests/integration/api_packages_test.go +++ b/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}) - - 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) {