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

ADD Github SSO for linking github account #259

Merged
merged 7 commits into from
Oct 1, 2021

Conversation

AlpacaMax
Copy link
Collaborator

This PR is the result of discussions in #240 . It's currently a work in progress. Details of this PR will be explained once all the changes I planned are committed and pushed.

A reminder to @nysteo is that in this PR the Profile page in front-end will be modified. Specifically the Github Username input box will likely be changed into a button that guides students to github authentication page.

@AlpacaMax
Copy link
Collaborator Author

@wabscale One thing that's different from the NYU OAuth is that in Github OAuth, we don't get the user information once we authenticated. Instead we need to use the access_token to request the github user api to retrieve the github username. Currently I did this API request in the backend code using requests library. However I think that's not a good practice as I assume it will add significantly more overhead to the server. A better approach would be to do this in the frontend code. What's your opinion on this?

@wabscale
Copy link
Collaborator

@wabscale One thing that's different from the NYU OAuth is that in Github OAuth, we don't get the user information once we authenticated. Instead we need to use the access_token to request the github user api to retrieve the github username. Currently I did this API request in the backend code using requests library. However I think that's not a good practice as I assume it will add significantly more overhead to the server. A better approach would be to do this in the frontend code. What's your opinion on this?

What you've done is completely reasonable. The NYU OAuth stuff we have requires us to connect out to auth.nyu.edu to get the student information too. It is fine for small requests like this. The overhead is not really something that we should worry too much about.

What we should do is add some kind of a try block around that request out to github so that if it fails, it doesn't just 500.

@wabscale
Copy link
Collaborator

We may also want to consider if we should remove the ability for the user to set their github username in this PR. I think maybe we could merge this now, and let teo and some of the others from the graduate team write the frontend piece for this. Then we could comfortably remove the ability for users to set their own github username, and force them to go through the github sso.

Regardless, outstanding work Max 👍

@wabscale wabscale added backend feature New feature or request labels Sep 29, 2021
@AlpacaMax
Copy link
Collaborator Author

We may also want to consider if we should remove the ability for the user to set their github username in this PR. I think maybe we could merge this now, and let teo and some of the others from the graduate team write the frontend piece for this. Then we could comfortably remove the ability for users to set their own github username, and force them to go through the github sso.

Regardless, outstanding work Max 👍

Yeah sure. I'll just wrap up tonight and let you merge this

@AlpacaMax
Copy link
Collaborator Author

I'll add the dynamic host selection thing tomorrow. My brain stops functioning at this point.
I think we can keep the old github username update endpoint for now before the deployment. It's a useful endpoint for testing purposes.

@wabscale
Copy link
Collaborator

Nice.

There is one last thing to handle here. If you are changing the name of the nyu environment variable, then you'll need to update the kubernetes yaml too. For the api deployment, that is here. That is where the oauth things are loaded from a kubernetes secret into an environment variable for the api.

@AlpacaMax
Copy link
Collaborator Author

I made all the changes I can do for now for this PR. Here are some of the things I think deserves some attention:

  1. The naming conventions for NYU consumer key and secret are changed. Please make corresponding name changes to the kubernetes secrets used in production. Also I don't know if they are only used in the authentication part of the api. Please make corresponding changes if these two are used in other parts of the application.
  2. Github client key and secret are introduced. Please register an oauth app on github and add them to the kubernetes secrets.
  3. Frontend code changes are required in the future to accomodate this PR. Simply add a button that visits https://anubis.osiris.services/api/public/github/link and remove the old github username input box. The old github username update endpoint can also be removed before deployment.
  4. Dynamic host selection is not added because the URL rule in anubis is a bit messy for this. The fact that an http proxy is used on the frontend makes it difficult to find a clean way to write this feature. As a result I think we should do this in a future PR.

@AlpacaMax AlpacaMax marked this pull request as ready for review September 30, 2021 19:55
@wabscale
Copy link
Collaborator

Hey @AlpacaMax this is good to go. I'm going to try to get you added as a collaborator to the repo so that you can click the button and get the commit in the history fully accredited to you. Will let you know 👍

@AlpacaMax
Copy link
Collaborator Author

Thank you so much! I'm super honored to be a collaborator!

@AlpacaMax AlpacaMax merged commit 87b8739 into AnubisLMS:master Oct 1, 2021
@wabscale wabscale linked an issue Oct 1, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADD github SSO for linking github account
2 participants