-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
def add_bearer_jwt_token(token, uri, headers, body, placement='header'): | ||
if placement in ('uri', 'url', 'query'): | ||
uri = add_to_uri(token, uri) | ||
elif placement in ('header', 'headers'): | ||
headers = add_to_headers(token, headers) | ||
elif placement == 'body': | ||
body = add_to_body(token, body) | ||
return uri, headers, body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we just need to modify the add_to_headers utility method, I think we could use the add_to_body and add_to_uri that authlib implements. Don't you think?
'last_name': '', | ||
'id': user_profile['preferred_username'], | ||
'username': user_profile['preferred_username'], | ||
'first_name': user_profile['family_name'] if user_profile['family_name'] else user_profile['name'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does user_profile always have the family_name
and name
entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have it backward:
'name': name,
'family_name': user.last_name,
'given_name': user.first_name,
'administrator': user.is_staff,
'superuser': user.is_superuser,
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/superset_config_docker.py
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
I tested this and it's working as expected. Just one quick comment, the first name and last name are recorded after registering, if the user already exists, then it's not saved again. I didn't know that. Maybe there's a configuration for updating existing users -- maybe it's not even needed since this project is brand new. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking fantastic, and really close to being done! Great work @Ian2012 and @mariajgrimaldi , this is so much cleaner.
Please see comments inline.
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
c1dacd6
to
105a38c
Compare
from collections import namedtuple | ||
import logging | ||
import MySQLdb | ||
|
||
from flask import current_app, session | ||
|
||
from superset.security import SupersetSecurityManager | ||
from superset.utils.memoized import memoized | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we run isort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
682e5d2
to
6931a5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this deployed in one of our installations, and it's working nicely! Thank you for the improvement 🥳
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
UserAccess = namedtuple( | ||
"UserAccess", ["username", "is_superuser", "is_staff"] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, we don't need this UserAccess
tuple (or the namedtuple
import) anymore:
UserAccess = namedtuple( | |
"UserAccess", ["username", "is_superuser", "is_staff"] | |
) |
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Outdated
Show resolved
Hide resolved
tutorsuperset/templates/superset/apps/pythonpath/openedx_sso_security_manager.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This works perfectly @Ian2012 ! Couple of minor nits noted below, but it's good to merge when they're resolved.
- I tested this on a fresh Tutor dev deployment.
- I read through the code
-
I checked for accessibility issuesN/A -
Includes documentationN/A
6931a5b
to
8922ee2
Compare
@pomegranited thanks for the review, it's corrected now |
@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR is an reimplementation of #40
This PR updates the oauth implementation to get a JWT to get the user information.
Have in mind that edx-platform doesn't store the fields first name and last name, that's why we need to provide the name base on the full name field. In the screenshot below, the user Cristhian G was logged in with the new implementation.
Also, the current implementation doesn't update the name after following logins.
This PR: