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

Overly generous GitHub Permissions #355

Closed
SimonCropp opened this Issue May 29, 2017 · 31 comments

Comments

Projects
None yet
9 participants
@SimonCropp
Copy link

SimonCropp commented May 29, 2017

These permissions would seem to be overly generous

image

Why are they required and can they be limited?

@xdamman

This comment has been minimized.

Copy link
Contributor

xdamman commented May 30, 2017

Agreed. We should just be able to read the contributors info, repos info (from user/org) and submit a pull request.

This might be a relevant previous commit on this topic: opencollective/opencollective-api@8fedf5b

Not sure what needs to be changed to reflect that. If you know, pull request very welcome! :)

/cc @asood123

@SimonCropp

This comment has been minimized.

@SimonCropp

This comment has been minimized.

Copy link
Author

SimonCropp commented Jun 3, 2017

not sure what u actually need from a repo? perhaps read:org and user:email would be a better combination? or even "only user:email" given u get "Grants read-only access to public information (includes public user profile info, public repository info, and gists)" by default?

@oliversauter

This comment has been minimized.

Copy link

oliversauter commented Jun 3, 2017

Hey folks,

I think reading everything will not be problematic for public repos. But reading private repos is not only very intense, but also does not seem to fit to the opencollective-spirit. Nobody would/could/should be able to attach a private repo to a collective anyhow.
So it seems as access to private repos is kinda unnecessary.

@xdamman

This comment has been minimized.

Copy link
Contributor

xdamman commented Jun 3, 2017

I agree. We don't need to be able to see/read private repos.

@SimonCropp

This comment has been minimized.

Copy link
Author

SimonCropp commented Jun 3, 2017

sounds good to me. i assume u dont need a PR for that :)

@SimonCropp

This comment has been minimized.

Copy link
Author

SimonCropp commented Jun 3, 2017

and i just re-read my initial issue text and realized it was overly blunt. sorry for that

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Jun 3, 2017

I've looked into this a couple of times and I can't find another scope that gives us what we need. Currently, we ask for: user:email, repo. We need to be able to list all repos that a user has write permissions to (even if they are part of an org) and then find other users who also have write permission on the repos that a user selects.

Happy to change if anyone has a smaller scope that gives us this info.

@SimonCropp

This comment has been minimized.

Copy link
Author

SimonCropp commented Jun 3, 2017

would the list of org memberships suffice? that seems to be public https://github.com/orgs/opencollective/people

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Jun 12, 2017

I don't think so. Opening that link when I'm logged into GitHub shows a much larger list than when I'm not. It would require each member to have made their membership public...

@SimonCropp

This comment has been minimized.

Copy link
Author

SimonCropp commented Jun 12, 2017

would require each member to have made their membership public.

Isnt this desired given the open nature opencollective is trying to promote.

But either way i can see here that you have done your due diligence in terms of trying to limit the permissions required. Sorry i forced you all to re-hash it. Happy for this to be closed. And thanks for taking he time to educate me

One note is perhaps it might be worth to capture the above context in a little document for the next person who is concerned?

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Jun 13, 2017

Yes, definitely desirable for everyone's membership to be public. Unfortunately, it's a manual action that each person needs to make per org that they are member of. Not everyone gets around to it and it ends up being unreliable info, which breaks our signup flow at times.

I'll figure out a way to document it. Or maybe we'll refer them to this issue when it comes up again ;-).

@asood123 asood123 closed this Jun 13, 2017

@fhunleth

This comment has been minimized.

Copy link

fhunleth commented Aug 13, 2017

I'm running into this issue as well since granting read/write permission to private repos is not something I can do. Not sure if this is a new option since when this issue was created, but couldn't the request for repo permissions be changed to public_repo?

@shiftkey

This comment has been minimized.

Copy link

shiftkey commented Oct 9, 2017

We need to be able to list all repos that a user has write permissions to (even if they are part of an org) and then find other users who also have write permission on the repos that a user selects.

I'm still not sure why the repo scope here is necessary over public_repo and org:read to achieve this. I have a little sample which I can share later that I used to poke at this using the github package, but here's the rough steps I used:

  • GET /users/teams to find all the teams a user belongs to. This include organizations. Each team has a privacy and permission setting here that might be of interest.
  • GET /teams/:id/repos gives you the details about which repositories the team has access to - this will have the permissions hash to indicate whether the team can push to it.
  • GET /teams/:id/members to then enumerate the other members in the team. This can be filtered on member or maintainer if you want to find specific groups of people.

Putting that aside, I'm also not sure about this comment from the linked commit:

 -      opts.scope = [ 'user:email', 'public_repo' ];
 +      // 'repo' gives us access to organizational repositories as well
 +      // vs. 'public_repo' which requires the org to give separate access to app
 +      opts.scope = [ 'user:email', 'repo' ]; 

Is this referring to OAuth app restrictions that can be enforced by organizations to only whitelist certain apps? There's documentation to request and approve to approve these apps in a given organization, which I think is what might need to happen.

@piamancini

This comment has been minimized.

Copy link
Contributor

piamancini commented Oct 9, 2017

cc/ @asood123

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Oct 12, 2017

Let me dig into this - need to context switch back to see how our flow is working and find test cases that were failing before.

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Oct 16, 2017

@shiftkey Ok I tried your steps. I get different results than what our current setup is:

Using your method, I looked up my own teams: came up with two. OpenCollective & xcodeswift (so far so good). Then, I used the team id to look up all the repos on my OpenCollective project. I get back 12 repos. It's missing lots of repos (and I'm an owner of team OpenCollective).

Currently, we use https://api.github.com/user/repos and when I run that for myself, I get over a 100 repos - most of them in OpenCollective.

To give you broader context, it's a balance between users complaining that their GitHub Repo isn't showing up on our sign up flow, so we broadened permissions. I'm happy to reduce them as long as we aren't confusing/losing users along the way.

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Oct 16, 2017

And here's the original commit when we changed it from public_repo after user feedback: opencollective/opencollective-api@8fedf5b

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Oct 16, 2017

I went through it one more time just to be safe. When I set permissions to public_repo, I get back 149 repos for me. When I set it to repo, I get back 166 - it includes public repositories from xcodeswift, where I'm a member but haven't made myself a public member (for no other reason except that it's the default). That's the issue we ran into when we set scope to public_repo. Many users were confused why their repo wasn't showing up in our flow.

I tried /user/orgs - only returns one org for me. /users/teams returns one org with two teams for me. Neither case includes xcodeswift org.

In ideal world, the GitHub permissions would make it simpler for users to choose repos across orgs and their own account. It's a tough trade off for us - either limit to public_repo and have many confused users (when they don't see their repos in the list) or lose users who aren't comfortable giving access to private repos (which we filter out btw).

I'm open to solutions - we would like to get a list of all repos that are public and that you have push permissions to. Main reason to try to include maximum number of repos is that GitHub projects are fluid in nature. People come and go all the time. So someone who may be the main contributor sometimes doesn't have access to the original org/team controls and can't make themselves an owner or make the org oauth accessible.

Hope this helps. Thanks for understanding.

@shiftkey

This comment has been minimized.

Copy link

shiftkey commented Oct 17, 2017

@asood123 thanks for the extra information. I tried a few things on my side to recreate the issue on my side, but don't have much to show for it currently.

I get back 12 repos. It's missing lots of repos (and I'm an owner of team OpenCollective).

This list is small because of how the organization is setup. This guide shows how you can view these repositories assigned to a team and add/remove them.

But if you're fine with only looking for public repositories, org:read, then /org/{name}/repos and the permissions hash on each repository might be Good Enough™ for this, rather than poking at the teams themselves.

When I set permissions to public_repo, I get back 149 repos for me. When I set it to repo, I get back 166 - it includes public repositories from xcodeswift, where I'm a member but haven't made myself a public member (for no other reason except that it's the default).

I can only see 12 public repositories on the xcodeswift org, so it's not clear where these other repositories are coming from. Let's assume they're private ones.

I tried /user/orgs - only returns one org for me. /users/teams returns one org with two teams for me. Neither case includes xcodeswift org.

This was working fine for me when I was using /user/orgs - I have a few orgs that I am hidden on:

  • unauthenticated, /user/shiftkey/orgs returns the six orgs you see on my profile
  • personal access token with org:read set, /user/orgs returns 33 orgs (i might be off by one, but whatever)

Anyway, to make it easier to investigate these differences in implementation I've built a test harness so we can easily diff these approaches for specific situations. Feel free to find specific cases so we can figure out which repositories are being overlooked and why. All you need to do is pass it a token (this should work with application authorizations too, but I haven't had a chance to try it): https://github.com/shiftkey/give-me-all-your-repos

I'm also still not clear on the comment in here about "requires the org to give separate access to app". Is there a back story here?

-      opts.scope = [ 'user:email', 'public_repo' ];
+      // 'repo' gives us access to organizational repositories as well
+      // vs. 'public_repo' which requires the org to give separate access to app
+      opts.scope = [ 'user:email', 'repo' ]; 
@jongalloway

This comment has been minimized.

Copy link

jongalloway commented Dec 8, 2017

This is marked closed, but doesn't look resolved. What's the status?

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Dec 8, 2017

Reopening this - we recently redid the GitHub flow and I think we can reduce permission scope.

@asood123 asood123 reopened this Dec 8, 2017

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Dec 13, 2017

Changing this back to public_repo to see if our new flow will reduce user issues that we hit earlier using public_repo. opencollective/opencollective-api#1059. Should go live later today.

@asood123 asood123 closed this Dec 13, 2017

@jongalloway

This comment has been minimized.

Copy link

jongalloway commented Dec 15, 2017

It looks like this is still requesting write access to public repos, which shouldn't be necessary.

Per this thread, from the reply by @shiftkey, it looks like you want to request "(no scope)" since public repos are already visible without requesting a scope.

(cc @leastprivilege)

@shiftkey

This comment has been minimized.

Copy link

shiftkey commented Dec 15, 2017

To clarify, "(no scope)" is the default when you don't specify any other scopes, and is documented here

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Dec 20, 2017

Updated: opencollective/opencollective-api#1067. Going live later today.

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Dec 21, 2017

We have two complaints in last 24 hours (since we pushed the change) that users aren't able to see their repos. It's likely the scope that's the issue. Reverting change to unblock those users while we figure out why this is happening.

@asood123

This comment has been minimized.

Copy link
Member

asood123 commented Dec 21, 2017

Both of those issues were resolved with public_repo scope. Seems GitHub API doesn't send all public repos without it. Open to alternatives, for now we'll stick to keeping that scope.

@shiftkey

This comment has been minimized.

Copy link

shiftkey commented Dec 22, 2017

We have two complaints in last 24 hours (since we pushed the change) that users aren't able to see their repos. It's likely the scope that's the issue.

Please reach out to GitHub support if you're encountering specific issues with using the API and would like some troubleshooting to understand these cases better...

@deamme

This comment has been minimized.

Copy link

deamme commented Jan 24, 2019

Can we open this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment