-
Notifications
You must be signed in to change notification settings - Fork 9
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
Authorize G3N Github app #67
Comments
The administration API seems to give access to a bunch of things but I agree that it's not obviously sensitive with just read access. I am curious - I don't see why the teams with access to a repo are needed for the app? Is there some reason for that access to be necessary? I think with the current state this is fine, though I would be concerned about future expansion of the APIs there into something sensitive. |
Hello! I helped to write the companion app. We use team lists on a repository to determine team membership, so that way if we get a PR that's assigned to a GitHub team, we can figure out what humans are on that team and notify them. The PR event the app sends doesn't come with that information. Also note that an app's developers can't just increase the permissions. If we were to change the app to use more permissions, then GitHub requires each user to re-install the app to allow those permissions. |
I think the issue is that if github adds new APIs to administration (or any other category), presumably you get those without further review, right? |
Yeah. We should provide Github with feedback about this structure. At this point I think they should avoid adding new, riskier APIs to administration and create a new permission for it instead. At the same time, I'm not sure what they would add.. I certainly would not expect to be able to read secrets or anything like that. |
Yeah, I'll try to include it in our next call with GitHub (presuming I remember). I think there's a bunch of semi-iffy APIs that could look reasonable to extend in a way that's not great (e.g., which specific accounts have branch protection override, to encourage attacks against those, something around security vulnerabilities - where just repository names/existence is leakage of some kind). But I think most of these are relatively innocuous in this case. I'll bring this up (if I remember!) at infra meeting next week, but I think at this point I'm not inclined to be too protective here, we can probably move ahead. |
I don't think granting access to a public repository also grants read-only "administration" access to all other public repositories in the org. I tried installing a test app in one of my personal repositories: I could access
|
G3 Notifications is a Chrome extension for Google employees to receive notifications when certain items need their attention, like code reviews, in a central place. It has a companion app that allows it to integrate with Github. The app is closed source.
The main benefit of enabling this would be improved responsiveness of Google employees to pull requests that require their review or that they either wrote.
This app would need to be authorized by a rust-lang admin to work with the Rust organization: https://github.com/apps/g3n-github/installations/new/permissions?target_id=5430905
See below for a discussion of the required permissions.
Repositories
The app only requires read access, and only public repositories are being requested. In an earlier Zulip thread @pietroalbini said:
You can accomplish this by picking the "Only select repositories" option and picking any public repo (say, rust-lang/rust).
Other public repos will be automatically included, according to the web page (but you must select at least one repo).EDIT: #67 (comment)Permissions
The app requests the following permissions:
I asked the developer about the administration permission, and got this response:
The link above has the set of APIs that are granted with this permission, which are worth looking over. Only read permission is required. I didn't see anything too concerning to me, but I also really wish Github didn't lump all these things together.
The text was updated successfully, but these errors were encountered: