-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Fix #6234 : Check organization visibility before everything else #6235
Fix #6234 : Check organization visibility before everything else #6235
Conversation
Hmm... I think we need to formally specify what the authorisation and permission regime should be. We're getting a lot of these little fixes and although I don't think they're ending up inconsistent, I suspect edge cases are likely to be missed and we may end up with security holes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repo.Owner may not be loaded - therefore you need to load the owner before dereferencing it.
That is:
repo.MustOwner()
Or use:
err:=repo.GetOwner()
And handle the error.
I hope I get it right. Having a well defined permission model would be nice - for example, it's not clear for me, if having public repositories in a private organization would make any sense. But an organization can freely switch between it's visibility settings, their repository should behave consistently after switching back and forth. |
Codecov Report
@@ Coverage Diff @@
## master #6235 +/- ##
==========================================
- Coverage 38.84% 38.83% -0.01%
==========================================
Files 355 355
Lines 50247 50256 +9
==========================================
+ Hits 19518 19519 +1
- Misses 27900 27907 +7
- Partials 2829 2830 +1
Continue to review full report at Codecov.
|
I like @zeripath 's idea. I think maybe someone could add that on the docs. |
But that's for another PR. @zeripath please review again. |
Fixing #6234