oss-aspen / 8Knot

Dash app in development to serve open source community visualizations using GitHub data from Augur. Hosted app: https://eightknot.osci.io
MIT License
47 stars 59 forks source link

add oauth w/ flask routes, serverside user groups #448

Closed JamesKunstle closed 11 months ago

JamesKunstle commented 12 months ago

This PR upgrades OAuth in 8Knot.

Previously, the entire OAuth flow was done in a callback. If the page URL changed, the callback would fire and would look for the 'auth_code' that the Augur OAuth server would provide. If the code was found, the callback would kick off the oauth flow, attempting to get a bearer_token / access_token from the Augur authorization server. User's groups would then be queried- in short, this callback was kludgey and not elegant.

This PR fixes up this logic- three new backend routes are available, /login, /authorize, and /logout, with user sessions handled by Flask-Login. /login redirects the user's browser to the Augur frontend, which redirects to /authorize upon UI authorization. /authorize does the required OAuth flow to get a bearer token which is cached in a user's session in redis, then redirects to the index route.

If the user is authorized via a present and valid session token, Dash kicks off the querying and caching of that user's groups from the Augur frontend.

-- Other improvements:

JamesKunstle commented 12 months ago

Waiting on Augur SSL to be updated.

JamesKunstle commented 12 months ago

@cdolfi The review for this PR will likely be better done as a conversation initially because of a few of the changes made. Please let me know if this is a feeling you share after giving it a once-over.

The biggest change is the removal of a big callback that handles oauth previously, and the switch to using Flask routes for all of that logic.

A small reorganization of files followed that package up the change reasonably.

cdolfi commented 12 months ago

@JamesKunstle Can you list all the files I should be actually reviewing/have changes and since there is such a file restructure can you rebase and make sure all new visualizations and changes are represented?

JamesKunstle commented 12 months ago

@cdolfi Yeah I'll rebase after the demo. Files that have changes should be annotated in the diff, those that were moved will just be grey.

JamesKunstle commented 12 months ago

fixes #262

JamesKunstle commented 12 months ago

@cdolfi PR is rebased and ready for review. Files for consideration have diffs in the review page.

JamesKunstle commented 11 months ago

@cdolfi I'm going to respond to the current review items then we I'll rebase the changes from #467

JamesKunstle commented 11 months ago

@cdolfi responded to requests- once you're satisfied with where this PR is in relation to completeness, I'll rebase on 'dev' so that we can merge. idk if you'll want to review that since it's just a merge.

cdolfi commented 11 months ago

@JamesKunstle merge on dev or main?

JamesKunstle commented 11 months ago

closing because changes were merged in #477