-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Implement third party auth ID mapping API #9842
Conversation
Thanks for the pull request, @xcompass! I've created OSPR-820 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here. |
ping @antoviaque |
Hi @xcompass - since this is a work in progress, could you let me know what resources, if any, you need from edX at this time? Could you also flesh out your PR description with more detail, especially regarding timeline (when do you need this feature by, and please be very explicit), how to test the feature, etc? Check out our detailed guidelines here: http://edx-developer-guide.readthedocs.org/en/latest/process/cover-letter.html - making specific note of our brand new guidelines on how to contribute to the documentation for your feature. |
@xcompass Thanks for the ping! We can schedule one of the two reviews on our side - likely over the coming week. |
@sarina I've updated the PR description. Let me know if I missed anything. Thanks. |
@sarina updated based on your comments. Is it easier for you to see the updates if I squash the comments? |
Separate commits are easier, I think. 👍 from me on test coverage and code quality. However, Braden and a platform team member will need to give thumbs up as well. |
@xcompass FYI, I will continue reviewing this for you once you add a model or mechanism for defining permissions that control access to this API (which is the missing piece needed for OAuth support, as we discussed). |
812c112
to
1aeb52f
Compare
@bradenmacdonald I've done the authorization part. Could you take a look at it when you get some time? Thanks! |
@nasthagiri I have some trouble to get authorization_code from command line (curl). What I'm doing right now is manually creating the code in Django admin. But once I use the code to get the token, the code is expired. (which is expected behaviour). Do you have any tip to get the authorization code from command line? Thanks. |
@xcompass To support server-to-server auth, we need to use the By the way, the |
@nasthagiri Thanks. I thought I missed something. Yep, I figured current django-oauth2-provider doesn't support client_credentials. It seems django-oauth2-toolkit supports client_credentials grant. I never used it before, so I'm not sure how much work involved. Are you guys interested in migrating to django-oauth2-toolkit? |
One thought is to address using the |
@nasthagiri @xcompass Is it possible for us to somehow use the |
@bradenmacdonald Yes, it's "possible". But it's not how the |
@nasthagiri I understand. I'm just trying to figure out how we can get this merged on time so that UBC has something they can use within their timeframe. Do you think we should use FYI @xcompass @nasthagiri @sarina I'm going to be away next week. So if we don't get this to the point where I can give +1 today, Eugeny will likely take over from me as a reviewer. |
@nasthagiri @bradenmacdonald This is what I'm doing when developing this authorization. |
Pan, that sounds good for now. We will then update edx-platform to support the client_credentials grant type. Thanks. Sent from my iPhone
|
@bradenmacdonald Updated based on your comments. |
45b793c
to
61a3bb6
Compare
@@ -52,6 +52,11 @@ def get(cls, provider_id): | |||
return None | |||
|
|||
@classmethod | |||
def get_all_provider_ids(cls): | |||
""" Gets a list of all enabled provider ids """ | |||
return [provider.provider_id for provider in cls._enabled_providers()] |
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.
Please remove this method (get_all_provider_ids
) and its test as well, if it's unused.
cf7875a
to
db6fcc5
Compare
@nasthagiri updated based on your comments. |
db6fcc5
to
a26722a
Compare
usernames_str = self.request.QUERY_PARAMS.get('usernames', None) | ||
remote_ids_str = self.request.QUERY_PARAMS.get('remote_ids', None) | ||
username_list = self.request.QUERY_PARAMS.getlist('username', None) | ||
remote_ids_list = self.request.QUERY_PARAMS.getlist('remote_id', None) |
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 presents a confusing API if there are multiple ways to provide a list of usernames and a list of remote_ids.
The QueryDict
implementation already supports both comma-separated and multi-value fields. Can we just support the last 2 queries above?
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.
Could you point me to where the QueryDict
supporting comma-separated fields? I can't find any mentioned comma in the doc. Also the field name is slightly different (username vs usernames). Should I use the same field name (singular form)? (/user?username=user1,user2)
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.
@xcompass Yes, please use the same field name: singular form.
I see the problem you are running into. In a separate (but not yet merged) PR, I have created a common MultiValueField class that serves exactly this purpose. It supports both ways of passing in multiple values for a filed.
That class is not yet merged and assumes one is using django Forms for parsing the query params. In the interim, though, you can use the same logic. Essentially:
- join all the fields into one comma-separated string, and then
- parse the comma-separated string into multiple values in a set/list.
@xcompass Thanks for updating the PR based on the last round of review. It looks great! I'm now waiting for the following items before giving my thumbs:
|
1eb4cbe
to
61e473f
Compare
61e473f
to
2bed346
Compare
@nasthagiri All done. Let me know if there is anything else. Looking forward to get this merge :) Thanks for all your help. |
2bed346
to
f9fcf41
Compare
({'username': [ALICE_USERNAME], 'remote_id': ['remote_' + STAFF_USERNAME]}, 200, | ||
get_mapping_data_by_usernames([ALICE_USERNAME, STAFF_USERNAME])), | ||
) | ||
@ddt.unpack |
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.
Are there any tests for testing invalid usernames and invalid remote_ids?
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.
Added
👍 |
This mapping API enables the mapping between the edX user ID and the ID provided by identity provider (IdP). For details, please see https://github.com/edx/edx-platform/pull/9842
f9fcf41
to
f693043
Compare
@sarina Nimisha finished the review. I have squashed the commits and rebased on current master. Let me know if there is anything else need to be done. |
@xcompass Looks good. Thanks for all your work on this PR. It's a great contribution to open edX and our partners. Is there anything left on your end? If not, I'll go ahead and merge. |
@nasthagiri I don't think so. Thanks. Glad to make the contribution and I learnt a lot :) |
Implement third party auth ID mapping API
FYI - @clintonb has added support for the |
@nasthagiri Great! I'll take a look at it and update the API in a new PR |
@nasthagiri It looks I don't need to update anything :) Is this in production already? (Edge and edx.org) I'll update the PR message to reflect the change. |
@xcompass It will be in production later today (hopefully by noon). You shouldn't need to update any code. However, you can (1) eliminate the use of the |
@nasthagiri Thanks! That's what I thought. |
UPDATE Feb.16, 2016
Since "client_credentials" grant type is supported know, there is no need to manually create authorization code. The workflow below reflects this change.
Description:
This implements a new API that can be used to retrieve a list of mappings between third party auth remote IDs and edX usernames. For use cases and more details, please see proposal + discussion.
Background:
When an edX course is used in an on-campus environment and SAML, LTI or other SSO is enabled to allow students to authenticate to edX using their on-campus credentials, normally there will be an identifier sent from an on-campus IdP. This identifier is stored in edX and linked to the edX account. Later when students return from an on-campus IdP, edX is able to authenticate the student using their edX account.
Because students may create edX accounts not using their real identity, the instructors will have difficulty to identify the edX students in their courses and won't be able to upload grades from edX back to on-campus gradebook system as the downloaded grades from edX only contain edX IDs, which are unknown to on-campus system.
This new API can be used for mapping between the edX user ID and the ID provided by identity provider (IdP). This API will give information that will allow instructors or course support staff to figure out the identity of the on-campus students who are using the edX course. It will also allow grade information coming from edX to be appropriately associated to campus students for uploading to an on-campus learning management system (LMS) or student information system (SIS).
This API is suppose to be consumed by a on-campus middleware, which combines the information from on-campus system. The middleware would talk to campus system to get information about course and enrolment. Then use those information to control instructor access. So the instructor will be able to get his/her edX student on-campus identity, uploading grades back to their LMS or SIS, making groups based on on-campus activities, etc.
This API also helps with troubleshooting with the issue where students put in wrong email addresses and weren't able to activate their accounts. Thus they weren't able to login EdX through Shibboleth because of the inactive accounts. With this API, on-campus course support staff or instructor can provide EdX support the student real EdX username instead of the ID that Shibboleth passed over to EdX, thus avoid the EdX support manually querying the tables to figure out the mapping between Shibboleth ID and EdX username.
JIRA: N/A
Component Affected: LMS (Third Party Auth)
Users Affected: Currently users with EDX_API_KEY (EdX internal) or OAuth2 client authorization (external use)
Timeframe: Mid Oct.
Reviewers: @bradenmacdonald and TBD
Resources Needed: None
Database Table Changes:
paver update_db
is required after merging.Test Instructions:
Create an authorization code. As EdX/django-auth2-provider doesn't support client_credential grant type, we need to create the authorization code manually. Go to OAuth2 -> Grants -> Add grant. Remember the code for later use.Using EDX_API_KEY
Using OAuth2
Using the authorization code created in the previous step to get the access token