Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that restricted users can access repos for which they are members #17460

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 27, 2021

There is a small bug in the way that repo access is checked in
repoAssignment: Accessibility is checked by checking if the user has a
marked access to the repository instead of checking if the user has any
team granted access.

This PR changes this permissions check to use HasAccess() which does the
correct test. There is also a fix in the release api ListReleases where
it should return draft releases if the user is a member of a team with
write access to the releases.

The PR also adds a testcase.

Fix #17003
Fix #17041

Signed-off-by: Andrew Thornton art27@cantab.net

There is a small bug in the way that repo access is checked in
repoAssignment: Accessibility is checked by checking if the user has a
marked access to the repository instead of checking if the user has any
team granted access.

This PR changes this permissions check to use HasAccess() which does the
correct test. There is also a fix in the release api ListReleases where
it should return draft releases if the user is a member of a team with
write access to the releases.

The PR also adds a testcase.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added this to the 1.16.0 milestone Oct 27, 2021
@zeripath zeripath linked an issue Oct 27, 2021 that may be closed by this pull request
6 tasks
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to reproduce the issue detailed in #17003, but the unit test and fix make sense.

Looks like the check before was only for AccessMode > 0, which didn't take into account UnitsMode.

To clarify, AccessMode is set when a user is given access in repo settings, while UnitsMode is set in team settings and handles permissions for each of the repo sections?

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 27, 2021
@zeripath
Copy link
Contributor Author

I wasn't able to reproduce the issue detailed in #17003, but the unit test and fix make sense.

If you just checkout the testcase without the rest of the changes Gitea will fail the test.

Looks like the check before was only for AccessMode > 0, which didn't take into account UnitsMode.

yup exactly that.

To clarify, AccessMode is set when a user is given access in repo settings, while UnitsMode is set in team settings and handles permissions for each of the repo sections?

Yes this appears to be the case.

@GiteaBot GiteaBot 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 Oct 28, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 28, 2021
…ers (go-gitea#17460)

Backport go-gitea#17460

There is a small bug in the way that repo access is checked in
repoAssignment: Accessibility is checked by checking if the user has a
marked access to the repository instead of checking if the user has any
team granted access.

This PR changes this permissions check to use HasAccess() which does the
correct test. There is also a fix in the release api ListReleases where
it should return draft releases if the user is a member of a team with
write access to the releases.

The PR also adds a testcase.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 28, 2021
@zeripath zeripath mentioned this pull request Oct 28, 2021
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@2b2eb5d). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #17460   +/-   ##
=======================================
  Coverage        ?   45.48%           
=======================================
  Files           ?      791           
  Lines           ?    88697           
  Branches        ?        0           
=======================================
  Hits            ?    40347           
  Misses          ?    41831           
  Partials        ?     6519           
Impacted Files Coverage Δ
modules/context/repo.go 54.03% <100.00%> (ø)
routers/api/v1/repo/release.go 44.69% <100.00%> (ø)

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 2b2eb5d...006c2f7. Read the comment docs.

@lunny lunny merged commit 0b4a8be into go-gitea:main Oct 28, 2021
lunny pushed a commit that referenced this pull request Oct 28, 2021
…ers (#17460) (#17464)

Backport #17460

There is a small bug in the way that repo access is checked in
repoAssignment: Accessibility is checked by checking if the user has a
marked access to the repository instead of checking if the user has any
team granted access.

This PR changes this permissions check to use HasAccess() which does the
correct test. There is also a fix in the release api ListReleases where
it should return draft releases if the user is a member of a team with
write access to the releases.

The PR also adds a testcase.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath deleted the fix-17003-restricted-user-access-repos branch October 28, 2021 07:00
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 28, 2021
Frontport go-gitea#17457

 ## [1.15.6](https://github.com/go-gitea/gitea/releases/tag/v1.15.6) - 2021-10-28

* BUGFIXES
  * Prevent panic in serv.go with Deploy Keys (go-gitea#17434) (go-gitea#17435)
  * Fix CSV render error (go-gitea#17406) (go-gitea#17431)
  * Read expected buffer size (go-gitea#17409) (go-gitea#17430)
  * Ensure that restricted users can access repos for which they are members (go-gitea#17460) (go-gitea#17464)
  * Make commit-statuses popup show correctly (go-gitea#17447) (go-gitea#17466)
* TESTING
  * Add integration tests for private.NoServCommand and private.ServCommand (go-gitea#17456) (go-gitea#17463)
zeripath added a commit that referenced this pull request Oct 28, 2021
Frontport #17457

 ## [1.15.6](https://github.com/go-gitea/gitea/releases/tag/v1.15.6) - 2021-10-28

* BUGFIXES
  * Prevent panic in serv.go with Deploy Keys (#17434) (#17435)
  * Fix CSV render error (#17406) (#17431)
  * Read expected buffer size (#17409) (#17430)
  * Ensure that restricted users can access repos for which they are members (#17460) (#17464)
  * Make commit-statuses popup show correctly (#17447) (#17466)
* TESTING
  * Add integration tests for private.NoServCommand and private.ServCommand (#17456) (#17463)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…ers (go-gitea#17460)

There is a small bug in the way that repo access is checked in
repoAssignment: Accessibility is checked by checking if the user has a
marked access to the repository instead of checking if the user has any
team granted access.

This PR changes this permissions check to use HasAccess() which does the
correct test. There is also a fix in the release api ListReleases where
it should return draft releases if the user is a member of a team with
write access to the releases.

The PR also adds a testcase.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Frontport go-gitea#17457

 ## [1.15.6](https://github.com/go-gitea/gitea/releases/tag/v1.15.6) - 2021-10-28

* BUGFIXES
  * Prevent panic in serv.go with Deploy Keys (go-gitea#17434) (go-gitea#17435)
  * Fix CSV render error (go-gitea#17406) (go-gitea#17431)
  * Read expected buffer size (go-gitea#17409) (go-gitea#17430)
  * Ensure that restricted users can access repos for which they are members (go-gitea#17460) (go-gitea#17464)
  * Make commit-statuses popup show correctly (go-gitea#17447) (go-gitea#17466)
* TESTING
  * Add integration tests for private.NoServCommand and private.ServCommand (go-gitea#17456) (go-gitea#17463)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
5 participants