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

Fixes #655 - Convert OAuth app to GitHub app to limit permissions we request #3629

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

ksy36
Copy link
Contributor

@ksy36 ksy36 commented Oct 28, 2021

I've created 3 GitHub apps, for production, staging and local (similarly to OAuth apps we have currently) and installed them on webcompat account. Each app has an access only to a specific repository (web-bugs for production app and webcompat-tests for staging and local).

The apps don't have any permissions apart from read and write access to issues of the repository where they're installed AND they request permission from users to act on their behalf through OAuth (again only on the repository where the app is installed). So the OAuth token that the app receives is valid only for a specific interaction (filing issues on web-bugs / webcompat-tests in our case).

There is an explanation of the process in https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/authorizing-github-apps#to-what-extent-can-a-github-app-know-which-resources-you-can-access--and-act-on-your-behalf

And this article was helpful too https://tyers.io/posts/4-github-apps (Github app vs OAuth app section).

The changes that will need to be made:

  1. Update GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET on the server to the one from GitHub app.
  2. Change SECRET_KEY on the server to invalidate existing sessions / logout users. This is needed so users don't try to submit issues with a token for the old OAuth app.
    When they log in again, they will be prompted to give access to the new GitHub app.
  3. Revoke tokens from the old OAuth app

@ksy36 ksy36 changed the title Fixes #655 - Convert oAuth app to GitHub app to limit permissions we request Fixes #655 - Convert OAuth app to GitHub app to limit permissions we request Oct 28, 2021
@ksy36 ksy36 marked this pull request as draft October 28, 2021 20:50
@@ -102,7 +103,7 @@ def login():
# manually set the referer so we know where to come back to
# when we return from GitHub
set_referer(request)
return github.authorize('public_repo')
return github.authorize()
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed as scopes are set during app creation/installation (and we don't really ask for any permissions apart from "act on your behalf").

@@ -319,7 +320,20 @@ def create_issue():
url_for('show_issue', number=json_response.get('number')))
# Authenticated reporting
if form.get('submit_type') == 'github-auth-report':
if g.user: # If you're already authed, submit the bug.
if g.user:
Copy link
Contributor Author

@ksy36 ksy36 Oct 31, 2021

Choose a reason for hiding this comment

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

This is to handle a case in the future when a user revokes a token, but is still logged in on webcompat.com and tries to submit an issue (not required for this migration to GitHub apps as the plan is to invalidate sessions to log out users, but nice to handle this exception).

@ksy36 ksy36 marked this pull request as ready for review October 31, 2021 16:11
@ksy36 ksy36 requested a review from karlcow October 31, 2021 16:12
@ksy36
Copy link
Contributor Author

ksy36 commented Oct 31, 2021

r? @karlcow

@@ -20,6 +20,7 @@
from flask import session
from flask import url_for
from flask_firehose import push
from flask_github import GitHubError
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts:

I wonder if this project is still maintained.
https://github.com/cenkalti/github-flask

Last PR on it was in 2020. and it is said to be compatible with Python 3.4
But I don't have any ideas without researching. This is not related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that too. We can revisit the use of this package in the near future. There are two alternatives that I found, https://github.com/singingwolfboy/flask-dance and authlib (https://docs.authlib.org/en/latest/client/flask.html#flask-client)

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

That looks so simple from the code point of view.

Thanks @ksy36 !
This is quite cool and one of the oldest requested feature.

@karlcow karlcow merged commit b5c9ae8 into main Nov 1, 2021
@miketaylr
Copy link
Member

Nice work @ksy36!

@miketaylr miketaylr deleted the issue/655/1 branch November 1, 2021 15:40
@ksy36
Copy link
Contributor Author

ksy36 commented Nov 1, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants