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

Minimize wpt-pr-bot's access levels #59

Open
foolip opened this issue Dec 11, 2018 · 12 comments
Open

Minimize wpt-pr-bot's access levels #59

foolip opened this issue Dec 11, 2018 · 12 comments
Assignees

Comments

@foolip
Copy link
Member

foolip commented Dec 11, 2018

Does this bot still perform any action that requires admin access to web-platform-tests/wpt?

If not, then I'd like to downgrade its access level to write access.

@zcorpan
Copy link
Member

zcorpan commented Dec 14, 2018

I think something with heroku needed it, but don't remember what. @tobie @jugglinmike ?

@foolip
Copy link
Member Author

foolip commented Feb 21, 2019

It previously would invite people as collaborators to the repo, but I removed that in #22.

If we downgrade the permissions to write access, are there logs that will reveal if something subtle breaks that used to work?

@jugglinmike
Copy link
Contributor

The best answer I can give is "I hope so." There's no question that we ought to be following POLP, though, so I'd like to help this move forward. Here's what I have in mind:

  • Improve integration tests
  • Define the feature set - In the absence of more comprehensive integration tests, that will help us build a little confidence that changes like this were acceptable.
  • Add installation instructions - This would allow someone to repeat the process of integrating with the WPT repository, and it would be a good place to specify exactly which "scopes" of the GitHub API are required

@foolip
Copy link
Member Author

foolip commented Feb 21, 2019

I have played around with https://github.com/features/actions a bit, enough to convince myself that both the merge_pr_* tagging and manifest upload we do on Travis and the wpt-pr-bot standalone service could be replaced by it. I don't know when it will exit beta, but I think that'd be a good time to move all this code over and test+document.

@foolip
Copy link
Member Author

foolip commented Nov 7, 2019

I spotted in https://github.com/web-platform-tests/wpt-pr-bot/settings/collaboration that @wpt-pr-bot actually has admin access to this very repo. I'm quite sure that's not necessary, so I've removed it.

@foolip
Copy link
Member Author

foolip commented Nov 7, 2019

This bit probably does require admin access to wpt:

function _status(handle) {
return github.get("/repos/:owner/:repo/collaborators/:username", { username: handle }).then(function() {
return github.get("/repos/:owner/:repo/collaborators/:username/permission", { username: handle }).then(function(data) {
return data.permission;
});
}, function() {
return "none";
});
}

@stephenmcgruer stephenmcgruer changed the title Does @wpt-pr-bot need admin access? Minimize wpt-pr-bot's access levels Feb 28, 2020
@stephenmcgruer stephenmcgruer self-assigned this Feb 28, 2020
@stephenmcgruer
Copy link
Contributor

Co-opting this to serve as the outcome of a recent retrospective, where we decided to investigate + lower the permissions given to wpt-pr-bot.

@stephenmcgruer
Copy link
Contributor

Summarizing current status, the wpt-pr-bot user:

  • Has access to the wpt repo with role admin.
  • Is part of the wpt.fyi team in the web-platform-tests org.
  • Has four access tokens defined: wpt-pr-bot (AppEngine instance), WPT "docs" branch management (building and deploying website), wpt.fyi, and web-platform-tests.
    • The first two of these were last used within the last week.
    • The wpt.fyi token was last used within the last 2 months.
    • The web-platform-tests token was last used within the last 3 months.

The tokens have varying levels of access:

  • wpt-pr-bot (AppEngine instance): public_repo, write:org, read:org
  • WPT "docs" branch management (building and deploying website): repo:status, repo_deployment, public_repo, repo:invite.
  • wpt.fyi: repo:status, repo_deployment, public_repo, repo:invite.
  • web-platform-tests: public_repo, write:org, read:org

@Hexcles
Copy link
Member

Hexcles commented Feb 28, 2020

wpt.fyi: repo:status, repo_deployment, public_repo, repo:invite.

This is no longer used. We've switched to wptfyibot. Please delete that token.

web-platform-tests: public_repo, write:org, read:org

Hmm what is this?

@Hexcles
Copy link
Member

Hexcles commented Feb 28, 2020

Also

Is part of the wpt.fyi team in the web-platform-tests org.

This can probably be removed, but I'm only 90% sure. It's best to do it next week and keep an eye in case anything breaks.

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Feb 28, 2020

This is no longer used. We've switched to wptfyibot. Please delete that token.

Done.

It's best to do it next week and keep an eye in case anything breaks.

Are you telling me not to make prod changes on a Friday evening? Pft, fineeeee :p

PS: Yes, the irony of the fact that I said that after happily deleting the wpt.fyi token on a Friday evening just hit me.

@foolip
Copy link
Member Author

foolip commented Mar 1, 2020

Is the admin role still needed? I think not after #22, but it's a bit tricky to audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants