Add endpoint to get SAML providers for a user. #18237

jcdyer · 2018-05-20T17:52:24Z

Adds an endpoint to allow mobile apps to get SSO providers for a given user. Includes throttling to prevent the endpoint from being used to crawl for logins. Due to policy concerns around potential leakage of user info, this is hidden behind a feature flag.

Upstreamed from edx#1047 and edx#1080

JIRA tickets: Implements OC-4285.

Dependencies: None

Screenshots:

Sandbox URL: https://pr18237.sandbox.opencraft.hosting/

Merge deadline: None

Testing instructions:

Set ALLOW_UNPRIVILEGED_SSO_PROVIDER_QUERY to true in lms.env.json.

Verify that the SSO user endpoint can be accessed by non-logged in users. Example: https://pr18237.sandbox.opencraft.hosting/api/third_party_auth/v0/users/staff

Verify that provider information can be retrieved by providing an email address. Example: https://pr18237.sandbox.opencraft.hosting/api/third_party_auth/v0/users/staff@example.com

Author notes and concerns:

Reviewers

@xitij2000
edX reviewer[s] TBD

Settings
Set ALLOW_UNPRIVILEGED_SSO_PROVIDER_QUERY to true in lms.env.json.

openedx-webhooks · 2018-05-20T17:52:31Z

Thanks for the pull request, @jcdyer! I've created OSPR-2428 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:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

mduboseedx · 2018-05-21T14:15:40Z

@jcdyer Feel free to ping me once this passes all the tests. Thanks!

jcdyer · 2018-05-21T16:35:01Z

@mduboseedx Thanks. I'm unable to find the diff quality report on the jenkins tests. Have those been removed or just relocated? It'd difficult to determine which issue is causing the quality build to fail without that information.

jcdyer · 2018-05-21T18:34:40Z

I think I found the source of the errors, (.coffee files that shouldn't have ben included), but I'm not sure how to diagnose that from the jenkins reports anymore.

mduboseedx · 2018-05-21T19:45:52Z

@edx/testeng Do you know why these quality test is failing? We're having trouble finding the diff quality report.

jmbowman · 2018-05-21T20:05:13Z

@mduboseedx It's the "diff_quality_pylint.html" artifact from the edx-platform-quality-diff sub-job: https://build.testeng.edx.org/job/edx-platform-quality-diff/2603/artifact/reports/diff_quality/diff_quality_pylint.html . Two unused imports.

jcdyer · 2018-05-21T20:51:00Z

Thanks @jmbowman.

xitij2000

👍

I tested this: ran the unit tests, tested in the devstack
I read through the code
[na] ~I checked for accessibility issues ~
[na] ~~Includes documentation~~

mduboseedx · 2018-05-23T14:12:24Z

@edx/platform-team Could you give this a review?

jcdyer · 2018-07-02T15:28:07Z

Ping! Can somebody from @edx/platform-team give this a look. This is upstreamed from work we are doing on edx-solutions/edx-platform.

jcdyer · 2018-07-09T20:39:25Z

@jmbowman Can you give this a review anytime soon, or find someone to do so?

macdiesel · 2018-07-10T13:06:08Z

@jcdyer This is on my radar now. I gave it a quick once over and it looks fine. If you can get tests passing I'll take a closer look and merge.

jcdyer · 2018-07-10T17:09:56Z

jenkins run bokchoy

mduboseedx · 2018-08-02T15:48:35Z

jenkins run bokchoy

mduboseedx · 2018-08-02T16:31:15Z

@jcdyer Have you had a chance to look into the failed bokchoy test?

doctoryes · 2018-09-11T22:04:12Z

jenkins run bokchoy

xitij2000 · 2018-09-12T09:35:47Z

jenkins run bokchoy

doctoryes · 2018-09-12T21:21:56Z

Looks like multiple failures in the bokchoy tests that are possibly legitimate. Those failures need to be addressed before this PR proceeds.

jcdyer · 2018-09-13T18:26:36Z

Thanks @doctoryes. I should be able to look into this again the week after next. I'll ping you when it's ready for another review.

jcdyer · 2018-09-24T19:48:48Z

@doctoryes I just rebased onto the latest master. Whatever the issue was, it seems to be resolved now.

jcdyer · 2018-09-25T18:20:09Z

Oh, sorry, I just realized I should at @macdiesel as well.

This is ready for a full review now.

macdiesel

Code looks good, just a few questions.

There are now tests to at least verify that the throttling rate is set correctly or works. We should add something here.

@pwnage101 is our on open source on call this week and will also review this.

macdiesel · 2018-09-27T13:05:34Z

common/djangoapps/third_party_auth/api/views.py

+    """
+    Maximum number of provider requests in a quick burst.
+    """
+    rate = '10/min'


Why not make this and the ProviderSustainedThrottle configuration settings?

macdiesel · 2018-09-27T13:08:32Z

common/djangoapps/third_party_auth/api/views.py

+            if '@' in username:
+                user = User.objects.get(email=username)
+            else:
+                user = User.objects.get(username=username)


@pwnage101 Don't we have a method that does this?

yes, use from student.models import get_user_by_username_or_email

pwnage101 · 2018-09-27T15:47:04Z

common/djangoapps/third_party_auth/api/views.py

 class UserView(APIView):
    """
    List the third party auth accounts linked to the specified user account.

    **Example Request**

        GET /api/third_party_auth/v0/users/{username}
+        GET /api/third_party_auth/v0/users/{email@example.com}


Can you explain why the username identifier is made into an ambiguous identifier here? In general, I think we were trying to phase out ambiguous identifiers whenever possible, so I have to ask when we're adding new ones.

Phase it out in favor of what?

Our client that we did the downstream work for had a requirement of allowing users to log in by either identifier.

Would you prefer to have different endpoints for each, or to use a query parameter to identify the type of identifier?

Yes, basically, phase out in favor of the unambiguous identifiers you recommended in your last sentence. Our original motivation might be selfish, but we ran into issues with ambiguous identifiers (combined with some of our usernames containing @ symbols) causing email lookups to contain usernames, and vice versa, which would fail to find an existing user.

Obviously, we do not allow usernames with @ symbols, and we still don't understand how it happened, but it "should not" happen so your code "should" be safe. I was just pushing back a little bit to see if making your endpoint unambiguous would still be okay.

Thanks @pwnage101. That's helpful information.

FYI @xitij2000 and @bradenmacdonald: This is related to the SSO provider lookup we did recently. This would require a minor change and the client side (having the api client choose an identifier type before making the request) and the server side (providing the argument as a query parameter) in the downstream code. Can we do that work to help edx move forward with unambiguous user identifiers?

I think the bigger concern would be mobile apps here. I think it should be a pretty small change on the server side.

However, it just pushes the logic of email vs username to whatever calls the API. If the @ symbol is allowed in usernames, or has ended up in usernames somehow, then whetever calls this API should call it twice if the @ symbol is present just to be sure. Would it make more sense for the API to do this automatically? i.e. find users that have the provided id as either a username or email.

We can't immediately change the API contract, because it would break the existing apps, but if we did want to push the "username vs. email" logic into the clients and make this API optionally unambiguous (by requiring a parameter/path to explicitly specify email vs. username), and later require it to be unambiguous, that would be fine.

I'll chime in here in summary: For this PR to proceed, we'd very much like it to include functionality to allow clients to unambiguously state whether they're sending an email address or a username, whether it's by two different new URLs -or- optional parameters on this endpoint. The old ambiguous endpoint would remain functional for now until clients can be shifted to use the unambiguous endpoint. But there'd be no remaining blocker to shifting clients over to use the unambiguous endpoint.

After clients are shifted, we'd then remove the old ambiguous endpoint functionality - perhaps in a new API version, if needed. Thoughts?

Sounds good to me. Here's how I'll implement that:

The proposed endpoint will remain. A new route will be added at /api/third_party_auth/v0/users/ (with no path segment for the ambiguous identifier), that takes one of ?username= or ?email=. If neither is provided, the endpoint will return a 400.

jcdyer · 2018-10-04T19:58:03Z

I will resolve review suggestions tonight or tomorrow morning.

jcdyer · 2018-10-13T12:12:04Z

@macdiesel, @pwnage101: Sorry for the delay. This is ready for another review now. I have added a new view with an explicit username / email query parameter, made throttles configurable via settings, and added tests for throttling.

pwnage101

looking pretty close!

pwnage101 · 2018-10-15T18:52:23Z

common/djangoapps/third_party_auth/api/urls.py

@@ -14,6 +14,7 @@
        UserView.as_view(),


I vaguely recall even wrapping this deprecated url pattern behind a feature toggle/django setting, but that may have been part of conversations with my team. @doctoryes @macdiesel what are your thoughts about making this togglable?

Oh, well we can't do that of course because it impacts an active endpoint. I'll just drop this concern for now.

pwnage101 · 2018-10-15T18:53:57Z

common/djangoapps/third_party_auth/api/views.py

    """
    List the third party auth accounts linked to the specified user account.

+    [DEPRECATED]


nit: add a sentence about renaming UserView2 to UserView after ripping this one out.

Better yet, I'll rename now, and call the old one UserViewDeprecated

On second thought, there could be extension libraries that are referring to this. Safer just to add the note.

pwnage101

LGTM. after additional review from our oncall and merge conflicts fixed.

bmedx

One comment otherwise looks good to me once it's rebased

bmedx · 2018-10-17T20:06:17Z

common/djangoapps/third_party_auth/api/views.py

+
+        Params must be a dict that includes only one of 'username' or 'email'
+        """
+        assert identifier.kind in {'username', 'email'}


Any reason not to use self.identifier_kinds here? Could you also change this to raise a ValueError instead of using an assert?

Updated, rebased, and squashed. Thanks!

@jcdyer I'm still seeing a conflict here. Can you take one more look?

You are correct. The rebase didn't take properly. Sorry about that!

jcdyer · 2018-10-18T18:01:17Z

Sorry. I rebased against the wrong branch, and then pushing didn't work. I'm resolving conflicts now.

View is combined with user SSO views. Includes a new version of the view that takes explicit "username" or "email". OC-4285

edx-status-bot · 2018-10-18T18:33:10Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2018-10-18T18:38:18Z

@jcdyer 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

bmedx · 2018-10-18T18:39:25Z

@jcdyer 🚢 thanks for your patience getting this through!

edx-pipeline-bot · 2018-10-18T20:11:57Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, October 19, 2018.

edx-pipeline-bot · 2018-10-19T19:00:36Z

EdX Release Notice: This PR has been deployed to the production environment.

pwnage101 · 2018-10-31T16:12:19Z

common/djangoapps/third_party_auth/api/views.py

-            }
-            for assoc in providers if assoc.has_account
-        ]
+# TODO: When removing deprecated UserView, rename this view to UserView.


Do we have an schedule estimate for addressing this tech debt?

I think our client would want to have at least one named release that includes the backwards-compatible code, and it could be removed any time after that.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 20, 2018

jcdyer force-pushed the cliff/sso-discovery-upstream branch 2 times, most recently from 03df339 to fa52e35 Compare May 21, 2018 00:47

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 21, 2018

xitij2000 approved these changes May 22, 2018

View reviewed changes

openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 23, 2018

jcdyer force-pushed the cliff/sso-discovery-upstream branch from ddd0390 to 26227e5 Compare July 10, 2018 17:23

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Sep 12, 2018

jcdyer force-pushed the cliff/sso-discovery-upstream branch from 26227e5 to 2db10f4 Compare September 24, 2018 19:01

macdiesel reviewed Sep 27, 2018

View reviewed changes

pwnage101 reviewed Sep 27, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-upstream branch 2 times, most recently from e195b14 to c491229 Compare October 13, 2018 12:09

jcdyer added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 15, 2018

pwnage101 reviewed Oct 15, 2018

View reviewed changes

pwnage101 approved these changes Oct 17, 2018

View reviewed changes

bmedx reviewed Oct 17, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-upstream branch from 14dd717 to 1f838c4 Compare October 18, 2018 17:58

Add endpoint to get SAML providers for a user.

b3521e0

View is combined with user SSO views. Includes a new version of the view that takes explicit "username" or "email". OC-4285

jcdyer force-pushed the cliff/sso-discovery-upstream branch from 1f838c4 to b3521e0 Compare October 18, 2018 18:02

bmedx merged commit 95ac59b into openedx:master Oct 18, 2018

pwnage101 reviewed Oct 31, 2018

View reviewed changes

openedx-webhooks added merged and removed engineering review labels Jan 22, 2021

bradenmacdonald deleted the cliff/sso-discovery-upstream branch January 12, 2022 01:30

Add endpoint to get SAML providers for a user. #18237

Add endpoint to get SAML providers for a user. #18237

Conversation

jcdyer commented May 20, 2018 • edited Loading

openedx-webhooks commented May 20, 2018

mduboseedx commented May 21, 2018

jcdyer commented May 21, 2018

jcdyer commented May 21, 2018

mduboseedx commented May 21, 2018

jmbowman commented May 21, 2018

jcdyer commented May 21, 2018

xitij2000 left a comment

Choose a reason for hiding this comment

mduboseedx commented May 23, 2018

jcdyer commented Jul 2, 2018

jcdyer commented Jul 9, 2018

macdiesel commented Jul 10, 2018

jcdyer commented Jul 10, 2018

mduboseedx commented Aug 2, 2018

mduboseedx commented Aug 2, 2018

doctoryes commented Sep 11, 2018

xitij2000 commented Sep 12, 2018

doctoryes commented Sep 12, 2018

jcdyer commented Sep 13, 2018

jcdyer commented Sep 24, 2018

jcdyer commented Sep 25, 2018

macdiesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwnage101 Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcdyer commented Oct 4, 2018

jcdyer commented Oct 13, 2018

pwnage101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwnage101 left a comment

Choose a reason for hiding this comment

bmedx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcdyer commented Oct 18, 2018

edx-status-bot commented Oct 18, 2018

openedx-webhooks commented Oct 18, 2018

bmedx commented Oct 18, 2018

edx-pipeline-bot commented Oct 18, 2018

edx-pipeline-bot commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcdyer commented May 20, 2018 •

edited

Loading

pwnage101 Sep 28, 2018 •

edited

Loading