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.

Sign up for GitHub

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

Jump to bottom

Restricting access to fork functioanlity to users with Code access #2534

Merged
merged 7 commits into from
Sep 18, 2017
Merged

Restricting access to fork functioanlity to users with Code access #2534

merged 7 commits into from
Sep 18, 2017

Conversation

jonasfranz
Copy link
Member

Actual situation: Users how do not have access to UnitTypeCode can create a fork of the repository and gain access to the code.

This fixes this problem by restiricting access to the fork functionality to users with UnitTypeCode.

@codecov-io
Copy link

codecov-io commented Sep 17, 2017 •
edited
Loading

Codecov Report

Merging #2534 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2534   +/-   ##
=======================================
  Coverage   26.96%   26.96%           
=======================================
  Files          84       84           
  Lines       16906    16906           
=======================================
  Hits         4559     4559           
  Misses      11672    11672           
  Partials      675      675
Impacted Files Coverage Δ
models/repo.go 13.53% <0%> (ø) ⬆️

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 6c6533f...6f744f7. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 17, 2017
@lafriks lafriks added this to the 1.3.0 milestone Sep 17, 2017
ethantkoenig
ethantkoenig suggested changes Sep 18, 2017
View reviewed changes
modules/context/repo.go Outdated
@@ -177,54 +177,72 @@ func RepoAssignment() macaron.Handler {
return func(ctx *Context) {
var (
owner *models.User
repo *models.Repository
Copy link
Member

Choose a reason for hiding this comment

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

Why are these changes to RepoAssignment() necessary? It seems to me that adding the context.LoadRepoUnits() and context.CheckUnit(models.UnitTypeCode) handlers should be sufficient (along with the changes to repo.CanBeForked())

Copy link
Member

Choose a reason for hiding this comment

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

I think it needs load repo because context.LoadRepoUnits() needs ctx.Reop to context but fork's route only has reopid no repopath.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, you're right.

But since we only need to load ctx.Repo, maybe it would be better to add a separate handler for loading :repoid, instead of making RepoAssignment() more complicated by adding a big if/else?

Copy link
Member

Choose a reason for hiding this comment

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

@ethantkoenig I agree with you to make a new middleware to do that but not change ReopAssignment

@jonasfranz
Adding a discrete function for RepoIDAssignment
f779a8f 
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@jonasfranz
Copy link
Member Author

@lafriks I think kind/security might also fit because of the illegal access to code.

@lafriks lafriks added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Sep 18, 2017
@lunny
Copy link
Member

lunny commented Sep 18, 2017

LGTM

@tboerger tboerger 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 Sep 18, 2017
@lafriks
Copy link
Member

lafriks commented Sep 18, 2017

LGTM

@tboerger tboerger 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 Sep 18, 2017
@appleboy
Copy link
Member

LGTM

@jonasfranz jonasfranz changed the title Fork permission bug fixes Restricting access to fork functioanlity to users with Code access Sep 18, 2017
@lunny lunny merged commit 566e8ec into go-gitea:master Sep 18, 2017
@lunny
Copy link
Member

lunny commented Sep 18, 2017

@JonasFranzDEV please send a back port to v1.2

@jonasfranz jonasfranz mentioned this pull request Sep 18, 2017
Merged
@jonasfranz
Copy link
Member Author

@lunny Done (#2542)

@lunny lunny added the backport/done All backports for this PR have been created label Sep 18, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@ethantkoenig ethantkoenig ethantkoenig requested changes

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
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. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/bug
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jonasfranz @codecov-io @lunny @lafriks @appleboy @ethantkoenig @tboerger