Clarify permission "HasAccess" behavior (#30585)

Follow #30495

"HasAccess" behavior wasn't clear, to make it clear:

* Use a new name `HasAnyUnitAccess`, it will be easier to review related
code and permission problems.
* Separate everyone access mode to a separate field, then all calls to
HasAccess are reverted to old behavior before #30495.
* Add new tests.

---------

Co-authored-by: Giteabot <teabot@gitea.io>

diff --git a/models/org_team.go b/models/org_team.go
index aecf0d80fd8acb696d67f36a5f8eb96dbc1cb818..b6908478c7ea5eba63d1c773d5e19b95a5414f00 100644 (file)
--- a/models/org_team.go
+++ b/models/org_team.go
@@ -118,7 +118,7 @@ func removeAllRepositories(ctx context.Context, t *organization.Team) (err error
 
                // Remove watches from all users and now unaccessible repos
                for _, user := range t.Members {
-                       has, err := access_model.HasAccess(ctx, user.ID, repo)
+                       has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo)
                        if err != nil {
                                return err
                        } else if has {
@@ -544,7 +544,7 @@ func ReconsiderRepoIssuesAssignee(ctx context.Context, repo *repo_model.Reposito
 }
 
 func ReconsiderWatches(ctx context.Context, repo *repo_model.Repository, user *user_model.User) error {
-       if has, err := access_model.HasAccess(ctx, user.ID, repo); err != nil || has {
+       if has, err := access_model.HasAnyUnitAccess(ctx, user.ID, repo); err != nil || has {
                return err
        }
        if err := repo_model.WatchRepo(ctx, user, repo, false); err != nil {

diff --git a/models/perm/access/access_test.go b/models/perm/access/access_test.go
index 79b131fe89643c1e3bc9a16bb2bfb2785939a925..51d625707c5bed87ca29e01fa1c54e16bf1ec63c 100644 (file)
--- a/models/perm/access/access_test.go
+++ b/models/perm/access/access_test.go
@@ -79,17 +79,17 @@ func TestHasAccess(t *testing.T) {
        repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
        assert.True(t, repo2.IsPrivate)
 
-       has, err := access_model.HasAccess(db.DefaultContext, user1.ID, repo1)
+       has, err := access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo1)
        assert.NoError(t, err)
        assert.True(t, has)
 
-       _, err = access_model.HasAccess(db.DefaultContext, user1.ID, repo2)
+       _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user1.ID, repo2)
        assert.NoError(t, err)
 
-       _, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo1)
+       _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo1)
        assert.NoError(t, err)
 
-       _, err = access_model.HasAccess(db.DefaultContext, user2.ID, repo2)
+       _, err = access_model.HasAnyUnitAccess(db.DefaultContext, user2.ID, repo2)
        assert.NoError(t, err)
 }
 

diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go
index 9cce95b77648ea6936d24a827829b962097671ce..0ed116a132465f450d546d8d24005003f9b64f26 100644 (file)
--- a/models/perm/access/repo_permission.go
+++ b/models/perm/access/repo_permission.go
@@ -24,6 +24,8 @@ type Permission struct {
 
        units     []*repo_model.RepoUnit
        unitsMode map[unit.Type]perm_model.AccessMode
+
+       everyoneAccessMode map[unit.Type]perm_model.AccessMode
 }
 
 // IsOwner returns true if current user is the owner of repository.
@@ -36,9 +38,24 @@ func (p *Permission) IsAdmin() bool {
        return p.AccessMode >= perm_model.AccessModeAdmin
 }
 
-// HasAccess returns true if the current user might have at least read access to any unit of this repository
-func (p *Permission) HasAccess() bool {
-       return len(p.unitsMode) > 0 || p.AccessMode >= perm_model.AccessModeRead
+// HasAnyUnitAccess returns true if the user might have at least one access mode to any unit of this repository.
+// It doesn't count the "everyone access mode".
+func (p *Permission) HasAnyUnitAccess() bool {
+       for _, v := range p.unitsMode {
+               if v >= perm_model.AccessModeRead {
+                       return true
+               }
+       }
+       return p.AccessMode >= perm_model.AccessModeRead
+}
+
+func (p *Permission) HasAnyUnitAccessOrEveryoneAccess() bool {
+       for _, v := range p.everyoneAccessMode {
+               if v >= perm_model.AccessModeRead {
+                       return true
+               }
+       }
+       return p.HasAnyUnitAccess()
 }
 
 // HasUnits returns true if the permission contains attached units
@@ -56,16 +73,16 @@ func (p *Permission) GetFirstUnitRepoID() int64 {
 }
 
 // UnitAccessMode returns current user access mode to the specify unit of the repository
+// It also considers "everyone access mode"
 func (p *Permission) UnitAccessMode(unitType unit.Type) perm_model.AccessMode {
-       if p.unitsMode != nil {
-               // if the units map contains the access mode, use it, but admin/owner mode could override it
-               if m, ok := p.unitsMode[unitType]; ok {
-                       return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m)
-               }
+       // if the units map contains the access mode, use it, but admin/owner mode could override it
+       if m, ok := p.unitsMode[unitType]; ok {
+               return util.Iif(p.AccessMode >= perm_model.AccessModeAdmin, p.AccessMode, m)
        }
        // if the units map does not contain the access mode, return the default access mode if the unit exists
+       unitDefaultAccessMode := max(p.AccessMode, p.everyoneAccessMode[unitType])
        hasUnit := slices.ContainsFunc(p.units, func(u *repo_model.RepoUnit) bool { return u.Type == unitType })
-       return util.Iif(hasUnit, p.AccessMode, perm_model.AccessModeNone)
+       return util.Iif(hasUnit, unitDefaultAccessMode, perm_model.AccessModeNone)
 }
 
 func (p *Permission) SetUnitsWithDefaultAccessMode(units []*repo_model.RepoUnit, mode perm_model.AccessMode) {
@@ -159,14 +176,15 @@ func (p *Permission) LogString() string {
 }
 
 func applyEveryoneRepoPermission(user *user_model.User, perm *Permission) {
-       if user != nil && user.ID > 0 {
-               for _, u := range perm.units {
-                       if perm.unitsMode == nil {
-                               perm.unitsMode = make(map[unit.Type]perm_model.AccessMode)
-                       }
-                       if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.unitsMode[u.Type] {
-                               perm.unitsMode[u.Type] = u.EveryoneAccessMode
+       if user == nil || user.ID <= 0 {
+               return
+       }
+       for _, u := range perm.units {
+               if u.EveryoneAccessMode >= perm_model.AccessModeRead && u.EveryoneAccessMode > perm.everyoneAccessMode[u.Type] {
+                       if perm.everyoneAccessMode == nil {
+                               perm.everyoneAccessMode = make(map[unit.Type]perm_model.AccessMode)
                        }
+                       perm.everyoneAccessMode[u.Type] = u.EveryoneAccessMode
                }
        }
 }
@@ -373,8 +391,8 @@ func CanBeAssigned(ctx context.Context, user *user_model.User, repo *repo_model.
                perm.CanAccessAny(perm_model.AccessModeRead, unit.TypePullRequests), nil
 }
 
-// HasAccess returns true if user has access to repo
-func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) {
+// HasAnyUnitAccess see the comment of "perm.HasAnyUnitAccess"
+func HasAnyUnitAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (bool, error) {
        var user *user_model.User
        var err error
        if userID > 0 {
@@ -387,7 +405,7 @@ func HasAccess(ctx context.Context, userID int64, repo *repo_model.Repository) (
        if err != nil {
                return false, err
        }
-       return perm.HasAccess(), nil
+       return perm.HasAnyUnitAccess(), nil
 }
 
 // getUsersWithAccessMode returns users that have at least given access mode to the repository.

diff --git a/models/perm/access/repo_permission_test.go b/models/perm/access/repo_permission_test.go
index aaa53bb24fede28c3f6d64c61a063a315decf93d..50070c436845b1c3cecdbd8fe44bc448a141c5da 100644 (file)
--- a/models/perm/access/repo_permission_test.go
+++ b/models/perm/access/repo_permission_test.go
@@ -14,16 +14,54 @@ import (
        "github.com/stretchr/testify/assert"
 )
 
+func TestHasAnyUnitAccess(t *testing.T) {
+       perm := Permission{}
+       assert.False(t, perm.HasAnyUnitAccess())
+
+       perm = Permission{
+               units: []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
+       }
+       assert.False(t, perm.HasAnyUnitAccess())
+       assert.False(t, perm.HasAnyUnitAccessOrEveryoneAccess())
+
+       perm = Permission{
+               units:              []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
+               everyoneAccessMode: map[unit.Type]perm_model.AccessMode{unit.TypeIssues: perm_model.AccessModeRead},
+       }
+       assert.False(t, perm.HasAnyUnitAccess())
+       assert.True(t, perm.HasAnyUnitAccessOrEveryoneAccess())
+
+       perm = Permission{
+               AccessMode: perm_model.AccessModeRead,
+               units:      []*repo_model.RepoUnit{{Type: unit.TypeWiki}},
+       }
+       assert.True(t, perm.HasAnyUnitAccess())
+
+       perm = Permission{
+               unitsMode: map[unit.Type]perm_model.AccessMode{unit.TypeWiki: perm_model.AccessModeRead},
+       }
+       assert.True(t, perm.HasAnyUnitAccess())
+}
+
 func TestApplyEveryoneRepoPermission(t *testing.T) {
        perm := Permission{
                AccessMode: perm_model.AccessModeNone,
                units: []*repo_model.RepoUnit{
-                       {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeNone},
+                       {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead},
                },
        }
        applyEveryoneRepoPermission(nil, &perm)
        assert.False(t, perm.CanRead(unit.TypeWiki))
 
+       perm = Permission{
+               AccessMode: perm_model.AccessModeNone,
+               units: []*repo_model.RepoUnit{
+                       {Type: unit.TypeWiki, EveryoneAccessMode: perm_model.AccessModeRead},
+               },
+       }
+       applyEveryoneRepoPermission(&user_model.User{ID: 0}, &perm)
+       assert.False(t, perm.CanRead(unit.TypeWiki))
+
        perm = Permission{
                AccessMode: perm_model.AccessModeNone,
                units: []*repo_model.RepoUnit{
@@ -40,8 +78,8 @@ func TestApplyEveryoneRepoPermission(t *testing.T) {
                },
        }
        applyEveryoneRepoPermission(&user_model.User{ID: 1}, &perm)
-       assert.True(t, perm.CanRead(unit.TypeWiki))
-       assert.False(t, perm.CanWrite(unit.TypeWiki)) // because there is no unit mode, so the everyone-mode is used as the unit's access mode
+       // it should work the same as "EveryoneAccessMode: none" because the default AccessMode should be applied to units
+       assert.True(t, perm.CanWrite(unit.TypeWiki))
 
        perm = Permission{
                units: []*repo_model.RepoUnit{

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index f60c5f21db2060037b45b09519abc781d8bfea9b..5358906f27d5120d5042b20813632406368a2022 100644 (file)
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -218,7 +218,7 @@ func repoAssignment() func(ctx *context.APIContext) {
                        }
                }
 
-               if !ctx.Repo.HasAccess() {
+               if !ctx.Repo.Permission.HasAnyUnitAccess() {
                        ctx.NotFound()
                        return
                }
@@ -412,7 +412,7 @@ func reqRepoReader(unitType unit.Type) func(ctx *context.APIContext) {
 // reqAnyRepoReader user should have any permission to read repository or permissions of site admin
 func reqAnyRepoReader() func(ctx *context.APIContext) {
        return func(ctx *context.APIContext) {
-               if !ctx.Repo.HasAccess() && !ctx.IsUserSiteAdmin() {
+               if !ctx.Repo.Permission.HasAnyUnitAccess() && !ctx.IsUserSiteAdmin() {
                        ctx.Error(http.StatusForbidden, "reqAnyRepoReader", "user should have any permission to read repository or permissions of site admin")
                        return
                }

diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
index 822e368fa84aea93527531499d175002f892e88b..2ac0b7ebd15628f7e0ad3ab7a5660ce83301e1c6 100644 (file)
--- a/routers/api/v1/repo/repo.go
+++ b/routers/api/v1/repo/repo.go
@@ -585,7 +585,7 @@ func GetByID(ctx *context.APIContext) {
        if err != nil {
                ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
                return
-       } else if !permission.HasAccess() {
+       } else if !permission.HasAnyUnitAccess() {
                ctx.NotFound()
                return
        }

diff --git a/routers/web/user/package.go b/routers/web/user/package.go
index 9af49406c4803c2db175ea9ac0d21a841afb8ed6..2a18796687363268ac3114f5977d90a12d6c70db 100644 (file)
--- a/routers/web/user/package.go
+++ b/routers/web/user/package.go
@@ -82,7 +82,7 @@ func ListPackages(ctx *context.Context) {
                        ctx.ServerError("GetUserRepoPermission", err)
                        return
                }
-               repositoryAccessMap[pd.Repository.ID] = permission.HasAccess()
+               repositoryAccessMap[pd.Repository.ID] = permission.HasAnyUnitAccess()
        }
 
        hasPackages, err := packages_model.HasOwnerPackages(ctx, ctx.ContextUser.ID)
@@ -276,7 +276,7 @@ func ViewPackageVersion(ctx *context.Context) {
                        ctx.ServerError("GetUserRepoPermission", err)
                        return
                }
-               hasRepositoryAccess = permission.HasAccess()
+               hasRepositoryAccess = permission.HasAnyUnitAccess()
        }
        ctx.Data["HasRepositoryAccess"] = hasRepositoryAccess
 

diff --git a/services/context/repo.go b/services/context/repo.go
index 1f4c698afc79c9749359cd066da268cb459e72bd..b17f99eb17b4f7775d1c3fd88d46eef243913190 100644 (file)
--- a/services/context/repo.go
+++ b/services/context/repo.go
@@ -374,8 +374,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) {
                return
        }
 
-       // Check access.
-       if !ctx.Repo.Permission.HasAccess() {
+       if !ctx.Repo.Permission.HasAnyUnitAccessOrEveryoneAccess() {
                if ctx.FormString("go-get") == "1" {
                        EarlyResponseForGoGetMeta(ctx)
                        return

diff --git a/services/convert/package.go b/services/convert/package.go
index b5fca21a3c3a55c02d61e0c2e4e37153b8582d46..b27992bea9e0276b14012aadaa49d6f15454f49b 100644 (file)
--- a/services/convert/package.go
+++ b/services/convert/package.go
@@ -21,7 +21,7 @@ func ToPackage(ctx context.Context, pd *packages.PackageDescriptor, doer *user_m
                        return nil, err
                }
 
-               if permission.HasAccess() {
+               if permission.HasAnyUnitAccess() {
                        repo = ToRepo(ctx, pd.Repository, permission)
                }
        }

diff --git a/services/repository/delete.go b/services/repository/delete.go
index 7c7dfe2dddb2b45f4920d10c814391d078a90bd9..cd779b05c3501bd41b695fcf934cc067dcb2afee 100644 (file)
--- a/services/repository/delete.go
+++ b/services/repository/delete.go
@@ -374,7 +374,7 @@ func removeRepositoryFromTeam(ctx context.Context, t *organization.Team, repo *r
                return fmt.Errorf("GetTeamMembers: %w", err)
        }
        for _, member := range teamMembers {
-               has, err := access_model.HasAccess(ctx, member.ID, repo)
+               has, err := access_model.HasAnyUnitAccess(ctx, member.ID, repo)
                if err != nil {
                        return err
                } else if has {

diff --git a/services/repository/transfer.go b/services/repository/transfer.go
index 83d303218825d20f6d1de5b33536398b9e55e0c1..3d0bce18d01d39034c3cde26d34253b89fa0d8d5 100644 (file)
--- a/services/repository/transfer.go
+++ b/services/repository/transfer.go
@@ -387,7 +387,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use
        }
 
        // In case the new owner would not have sufficient access to the repo, give access rights for read
-       hasAccess, err := access_model.HasAccess(ctx, newOwner.ID, repo)
+       hasAccess, err := access_model.HasAnyUnitAccess(ctx, newOwner.ID, repo)
        if err != nil {
                return err
        }

diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go
index c3f03d6638f607faaf3d6c9f1fbb30bf2bb36c25..67799eddcc361971f413845d41868663b96f1f3f 100644 (file)
--- a/services/repository/transfer_test.go
+++ b/services/repository/transfer_test.go
@@ -67,13 +67,13 @@ func TestStartRepositoryTransferSetPermission(t *testing.T) {
        repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
        repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
-       hasAccess, err := access_model.HasAccess(db.DefaultContext, recipient.ID, repo)
+       hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo)
        assert.NoError(t, err)
        assert.False(t, hasAccess)
 
        assert.NoError(t, StartRepositoryTransfer(db.DefaultContext, doer, recipient, repo, nil))
 
-       hasAccess, err = access_model.HasAccess(db.DefaultContext, recipient.ID, repo)
+       hasAccess, err = access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo)
        assert.NoError(t, err)
        assert.True(t, hasAccess)
 

diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go
index 481732f8df4462b44644367868c87a8129ec5672..bc2720d51e057422b109e58c1368c726aa86632d 100644 (file)
--- a/tests/integration/api_repo_test.go
+++ b/tests/integration/api_repo_test.go
@@ -222,7 +222,7 @@ func TestAPISearchRepo(t *testing.T) {
                                        assert.Len(t, repoNames, expected.count)
                                        for _, repo := range body.Data {
                                                r := getRepo(t, repo.ID)
-                                               hasAccess, err := access_model.HasAccess(db.DefaultContext, userID, r)
+                                               hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, userID, r)
                                                assert.NoError(t, err, "Error when checking if User: %d has access to %s: %v", userID, repo.FullName, err)
                                                assert.True(t, hasAccess, "User: %d does not have access to %s", userID, repo.FullName)