Restrict permission check on repositories and fix some problems #5314

lunny · 2018-11-11T02:31:08Z

This PR will refactor permission check of repositories, especially units permission check. It will restrict permission check on organization repositories and fix some bugs. It creates a new struct Permission and a function to retrieve permission from User and Repository.

You can know all the permission information from Permission and it has been saved on context.Context when middleware repoAssignment was called. After this PR, we removed some functions like IsRepositoryWriter because we don't know that if the repo is on an organization and there are some teams give permission to the user. Instead it provides CanAccess(unit) and CanWrite(unit) to determine if the User could read or write to code, issues, releases or other units.

This also give a break change about a private repository on an organization. When a user on a team has write permission to a repository but he also is a collaborator with read right to that repository. Before this PR, the team settings will be ignored, but in this PR, the higher permission will be given to the user. Don't know it's a bug or a break change. I added the breaking label.

techknowlogick

I didn't check all files, but Gitea copyright needs to be added in some files.

routers/api/v1/repo/collaborators.go

routers/api/v1/api.go

routers/api/v1/repo/file.go

routers/api/v1/repo/issue_label.go

routers/api/v1/repo/label.go

zeripath · 2018-11-11T17:14:28Z

models/repo_permission.go

+ }
+
+ perm.Units = make([]*RepoUnit, 0, len(repo.Units))
+ for t, _ := range perm.UnitsMode {


Lint doesn't like the second argument

zeripath · 2018-11-11T17:15:39Z

models/repo_permission.go

+
+ // Collaborators on organization repos will not be limited by teams units
+ if isCollaborator, err := repo.isCollaborator(e, user.ID); err != nil {
+ return perm, err


All of the other returns rely on the default named return values.

Because this err is a local variable not that err return variable.

zeripath · 2018-11-11T17:15:51Z

models/repo_permission.go

+ if isCollaborator, err := repo.isCollaborator(e, user.ID); err != nil {
+ return perm, err
+ } else if isCollaborator {
+ return perm, nil


zeripath · 2018-11-11T17:33:02Z

routers/api/v1/api.go

@@ -1,4 +1,5 @@
 // Copyright 2015 The Gogs Authors. All rights reserved.
+// Copyright 2016 The Gitea Authors. All rights reserved.


I think maybe 2016 because the file might have been modified then.

@techknowlogick @zeripath yes. If I explicitly know the file is modified on some year I will change to that year, but will change it to 2018.

codecov-io · 2018-11-12T07:37:53Z

Codecov Report

Merging #5314 into master will increase coverage by 0.02%.
The diff coverage is 49.76%.

@@            Coverage Diff             @@
##           master    #5314      +/-   ##
==========================================
+ Coverage   37.56%   37.59%   +0.02%     
==========================================
  Files         313      315       +2     
  Lines       46611    46714     +103     
==========================================
+ Hits        17510    17560      +50     
- Misses      26607    26657      +50     
- Partials     2494     2497       +3

Impacted Files	Coverage Δ
routers/api/v1/repo/release.go	`47.9% <ø> (+2.09%)`	⬆️
models/repo_unit.go	`60.71% <ø> (+2.09%)`	⬆️
routers/api/v1/repo/file.go	`22.8% <ø> (+0.77%)`	⬆️
models/user.go	`45.42% <ø> (+0.58%)`	⬆️
models/access.go	`54.9% <ø> (-5.73%)`	⬇️
routers/api/v1/repo/label.go	`0% <ø> (ø)`	⬆️
routers/api/v1/repo/collaborators.go	`0% <ø> (ø)`	⬆️
models/branches.go	`47.32% <0%> (-1.7%)`	⬇️
routers/api/v1/repo/pull.go	`17.13% <0%> (-0.16%)`	⬇️
routers/api/v1/repo/fork.go	`0% <0%> (ø)`	⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0222623...d54bc51. Read the comment docs.

kTitan · 2018-11-17T22:55:15Z

Does this PR also fix #4329?

lunny · 2018-11-18T02:07:00Z

Just place a FIXME and another PR could change that function. This PR will only refactor or fix permissions check.

lunny · 2018-11-18T11:02:07Z

I think this is ready for review

lunny · 2018-11-25T05:18:28Z

rebased

lunny · 2018-11-27T11:18:14Z

@zeripath need your approval.

lafriks · 2018-11-27T23:19:04Z

@lunny please resolve conflicts

lunny · 2018-11-28T03:11:03Z

@lafriks done.

zeripath · 2018-11-28T09:25:05Z

Sorry @lunny I've been a little busy so haven't been able to keep up with your changes.

lunny added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Nov 11, 2018

lunny added this to the 1.7.0 milestone Nov 11, 2018

techknowlogick requested changes Nov 11, 2018

View reviewed changes

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 11, 2018

zeripath requested changes Nov 11, 2018

View reviewed changes

lunny changed the title ~~WIP: Fix units permission problems~~ WIP: Fix permission check problems Nov 13, 2018

lunny changed the title ~~WIP: Fix permission check problems~~ WIP: Restrict permission check on repositories and fix some problems Nov 13, 2018

lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Nov 13, 2018

lunny force-pushed the lunny/fix_units_permissions branch from e518b8f to 2070b5c Compare November 17, 2018 05:28

lunny changed the title ~~WIP: Restrict permission check on repositories and fix some problems~~ Restrict permission check on repositories and fix some problems Nov 18, 2018

This was referenced Nov 18, 2018

Allow to set organization visibility (public, internal, private) #1763

Merged

Implement git refs API for listing references (branches, tags and other) #5354

Merged

lunny force-pushed the lunny/fix_units_permissions branch from ec36aee to f90dc26 Compare November 25, 2018 05:17

techknowlogick approved these changes Nov 26, 2018

View reviewed changes

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 26, 2018

lafriks approved these changes Nov 27, 2018

View reviewed changes

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 27, 2018

lunny added 3 commits November 28, 2018 11:07

fix units permission problems

0bf41c2

fix some bugs and merge LoadUnits to repoAssignment

15f80b9

refactor permission struct and add some copyright heads

40d552c

lunny added 12 commits November 28, 2018 11:09

fix tests

a978acc

fix tests

d161315

fix tests

e5e165c

fix AccessLevel

9253015

rename CanAccess

2f65f7a

fix tests

962be78

fix comment

9742d63

fix bug

2db05db

add missing unit for test repos

3620109

fix bug

861b3b2

rename some functions

b677d04

fix routes check

79365d8

lunny force-pushed the lunny/fix_units_permissions branch from f90dc26 to 79365d8 Compare November 28, 2018 03:10

zeripath approved these changes Nov 28, 2018

View reviewed changes

Merge branch 'master' into lunny/fix_units_permissions

d54bc51

This was referenced Nov 28, 2018

Write / Read Permissions for specific units are ignored #2687

Closed

Organizations: compromise team read access by another team with write access #3135

Closed

lunny merged commit eabbddc into go-gitea:master Nov 28, 2018

lunny deleted the lunny/fix_units_permissions branch November 28, 2018 11:27

This was referenced Dec 8, 2018

Pull Requests can be listed without having the corresponding permission #4587

Closed

fix adding reaction fail for read permission user #5515

Merged

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict permission check on repositories and fix some problems #5314

Restrict permission check on repositories and fix some problems #5314

lunny commented Nov 11, 2018 •

edited

techknowlogick left a comment

zeripath Nov 11, 2018

lunny Nov 12, 2018

zeripath Nov 11, 2018

lunny Nov 12, 2018

zeripath Nov 11, 2018

zeripath Nov 11, 2018

techknowlogick Nov 11, 2018

zeripath Nov 11, 2018

lunny Nov 12, 2018

codecov-io commented Nov 12, 2018 •

edited

kTitan commented Nov 17, 2018

lunny commented Nov 18, 2018

lunny commented Nov 18, 2018

lunny commented Nov 25, 2018

lunny commented Nov 27, 2018

lafriks commented Nov 27, 2018

lunny commented Nov 28, 2018

zeripath commented Nov 28, 2018

		@@ -1,4 +1,5 @@
		// Copyright 2015 The Gogs Authors. All rights reserved.
		// Copyright 2016 The Gitea Authors. All rights reserved.

Restrict permission check on repositories and fix some problems #5314

Restrict permission check on repositories and fix some problems #5314

Conversation

lunny commented Nov 11, 2018 • edited

techknowlogick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 12, 2018 • edited

Codecov Report

kTitan commented Nov 17, 2018

lunny commented Nov 18, 2018

lunny commented Nov 18, 2018

lunny commented Nov 25, 2018

lunny commented Nov 27, 2018

lafriks commented Nov 27, 2018

lunny commented Nov 28, 2018

zeripath commented Nov 28, 2018

lunny commented Nov 11, 2018 •

edited

codecov-io commented Nov 12, 2018 •

edited