From bbf83f5d4bd8dbe1cd6dbcf7b45ef47072e5add0 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Thu, 6 Apr 2023 23:18:29 +0900 Subject: [PATCH] Improve permission check of packages (#23879) At first, we have one unified team unit permission which is called `Team.Authorize` in DB. But since https://github.com/go-gitea/gitea/pull/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 --- models/fixtures/org_user.yml | 6 ++++++ models/fixtures/team.yml | 13 +++++++++++- models/fixtures/team_unit.yml | 6 ++++++ models/fixtures/team_user.yml | 6 ++++++ models/fixtures/user.yml | 4 ++-- models/organization/org_test.go | 16 ++++++++++----- modules/context/package.go | 28 +++++++++----------------- tests/integration/api_packages_test.go | 10 +++++++++ 8 files changed, 63 insertions(+), 26 deletions(-) diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index d6bbdaa9d..d08f69579 100644 --- a/models/fixtures/org_user.yml +++ b/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 diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index be988b8fc..aa3b36e64 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -172,4 +172,15 @@ num_repos: 0 num_members: 0 includes_all_repositories: false - can_create_org_repo: true \ No newline at end of file + 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 diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index 2e23a6312..5257d2c38 100644 --- a/models/fixtures/team_unit.yml +++ b/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 diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index de4f29d97..b95f76c72 100644 --- a/models/fixtures/team_user.yml +++ b/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 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index ce54defac..3e302dfb9 100644 --- a/models/fixtures/user.yml +++ b/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: "" diff --git a/models/organization/org_test.go b/models/organization/org_test.go index cfa304d7b..6e5838799 100644 --- a/models/organization/org_test.go +++ b/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) diff --git a/modules/context/package.go b/modules/context/package.go index 2a55db3a7..2a0159eb5 100644 --- a/modules/context/package.go +++ b/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 { diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index 4228003e2..74a7e3c79 100644 --- a/tests/integration/api_packages_test.go +++ b/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) {