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

jcdyer · 2018-04-10T21:05:55Z

This PR contains a new view that returns a list of authentication providers for a user. This includes SAML, OAuth, and any other configured 3rd party auth providers.

JIRA tickets: Implements OC-4285, MCKIN-7245

Dependencies: None

Screenshots: None

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: None

Testing instructions:

As an unauthenticated user, access /api/third_party_auth/v0/users//providers/ for a user with at least one third-party-auth connection. Verify that the providers are included in the response.
Replace with the same user's email address, and verify that the same response is returned.
Replace with the username or email address of a user that doesn't have any third-party-auth connections. Verify the the returned list of providers is empty.
Replace with a nonexistent username. Verify that a 200 response is returned with an empty list of providers.
Run third-party-auth unittests: paver test_system -s lms -t common/djangoapps/third_party_auth/api/tests/
Hit the api several times (11) in one minute. Verify that a 429 (throttled) response is eventually returned. If you're feeling ambitious, send 51 requests over the course of 10 minutes, and verify that the daily throttle limit gets hit as well.

Author notes and concerns:

~~Throttling seems to be happening, but not according to the configuration I provided. Tested via curl on the command line.~~ Throttling works now.
Tests are failing due to NPM installation issues. I spent a few hours trying to resolve the problem this morning, with no luck. I'll ask around if anyone else has faced this, and if not I'll try to deal with it later. In the meantime, please review with manual testing, and also run unit tests manually using paver test_system -s lms -t common/djangoapps/api/tests

Reviewers:

@xitij2000

jcdyer · 2018-04-11T15:47:12Z

Accidentally closed PR.

jcdyer · 2018-04-12T12:57:51Z

Had to fix JSLint issues. How did those get in there?

xitij2000

I'm working on testing this manually but noticed a few issues that you could address. Further feedback once I've tested the code a bit.

xitij2000 · 2018-04-11T20:30:31Z

circle.yml

@@ -11,7 +11,11 @@ dependencies:
  pre:
    - bash ./scripts/install-system-req.sh
  override:


I checked, and each command in override runs in its own shell instance, so nvm use will not work since it makes changes to the active shell and those aren't preserved for the next command.

xitij2000 · 2018-04-19T18:19:14Z

common/djangoapps/third_party_auth/api/tests/test_views.py

+    """
+    Test that the provider endpoint returns the appropriate data
+    """
+    def test_results(self):


Could you split up this into multiple tests for linked and unlinked users? Also, you can use the ddt instead of a loop.

xitij2000 · 2018-04-19T18:25:47Z

common/djangoapps/third_party_auth/api/views.py

+
+            * provider_id: The unique identifier of this provider (string)
+            * name: The name of this provider (string)
+            * remote_id: The ID of the user according to the provider. This ID


This documentation does not seem to be up to date with the code.

jcdyer · 2018-04-19T19:00:27Z

Addressed feedback provided so far.

jcdyer · 2018-04-19T19:01:00Z

Currently waiting on circle-ci and vagrant so I can confirm the fix does what it should.

xitij2000 · 2018-04-19T19:47:52Z

@jcdyer Thanks! I tested it manually and it seems to be working as expected, and throttling is working as well. I also ran the new tests (post ddt refactor) and they are passing.
I just don't know about the JavaScript changes, are they still necessary? I did not see these issues when testing locally with the JavaScript changes removed.

jcdyer · 2018-04-20T20:42:51Z

Trying a test run without the jslint fixes.

xitij2000 · 2018-04-20T20:44:45Z

@jcdyer Thanks! I think the main code looks good to merge now, but some unrelated tests seem to be failing.

jcdyer · 2018-04-20T20:46:57Z

Yep. I tried rebasing. If that doesn't solve it, I'll bump the query numbers, but I'd rather understand why the query count increased.

xitij2000 · 2018-04-21T08:52:36Z

@jcdyer It seems those test and quality failures are to be expected and being worked on separately, so as long as those are the only failures this is good to go.

xitij2000 · 2018-04-21T09:11:23Z

I did another test with all the latest changes and the relevant tests are passing, so I think this is close to done. Please just update the code documentation and update the ticket description, and we can merge this.

jcdyer · 2018-04-21T15:34:49Z

Docs were updated. Did I miss another place where they were out of date? (I just squashed commits. That may have been unhelpful. Sorry about that.) The ticket description looks accurate on OC- and I don't have access to MCKIN-, so I don't know if that should be updated or not. Perhaps @bradenmacdonald can assist. Once you update your review to Approval, we should be able to merge.

xitij2000 · 2018-04-21T15:44:19Z

@jcdyer Sorry I meant PR description. I currently contains the text: "Description goes here. e.g. This PR contains the LibraryContent XBlock, which allows to display library content in a course."

As for docs I mean the get method currently includes:

HTTP Endpoint for all CRUD operations for a user course enrollment. Allows creation, reading, and
updates of the current enrollment for a particular course.

xitij2000

👍

I tested this:
- I ran the automated tests
- I manually tested the API response to ensure it matches what we need
- I manually tested that the throttling works (see note below)
I read through the code
~~I checked for accessibility issues~~
Includes documentation (see note below)

Please update the get method docstring, I think that is the only thing that's missing now.

While throttling is working for me, it is not working the way I expected it, or as described in the ticket. For me, only 5 requests per second are allowed, and the 50/day limit isn't working. I'm still looking into this since I think it's an issue on my side.

xitij2000 · 2018-04-21T09:00:54Z

common/djangoapps/third_party_auth/api/views.py

+    throttle_classes = [ProviderSustainedThrottle, ProviderBurstThrottle]
+
+    def get(self, request, identifier):
+        """Create, read, or update enrollment information for a user.


Please update these docs as well.

jcdyer · 2018-04-23T13:58:27Z

Updated docs and PR description. Commits squashed & waiting for CI.

xitij2000 · 2018-04-23T14:02:40Z

@jcdyer Thanks! The CI tests will probably fail again though for the same reasons as before.

OC-4285

jcdyer · 2018-04-25T18:22:18Z

Confirmed. The only failures are the quality issues now. (Query counts are back to normal after the latest rebase.

jcdyer force-pushed the cliff/sso-discovery-endpoint branch 3 times, most recently from a6938b9 to 434dad4 Compare April 11, 2018 15:17

jcdyer closed this Apr 11, 2018

jcdyer reopened this Apr 11, 2018

jcdyer force-pushed the cliff/sso-discovery-endpoint branch 2 times, most recently from ed4b177 to 09b26f7 Compare April 12, 2018 12:05

jcdyer changed the base branch from master to integration April 12, 2018 12:25

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from 255e9ba to 291ba07 Compare April 12, 2018 12:30

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from f17ee35 to c7bf542 Compare April 12, 2018 18:26

xitij2000 suggested changes Apr 19, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from 3ad91f1 to fc08bfa Compare April 20, 2018 20:42

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from ea4337b to db14792 Compare April 21, 2018 15:29

xitij2000 approved these changes Apr 22, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from db14792 to 6d46c61 Compare April 23, 2018 13:57

Add endpoint to get SAML providers for a user.

880d796

OC-4285

jcdyer force-pushed the cliff/sso-discovery-endpoint branch from 6d46c61 to 880d796 Compare April 23, 2018 17:08

jcdyer merged commit 3a00060 into integration Apr 25, 2018

xitij2000 deleted the cliff/sso-discovery-endpoint branch April 25, 2018 22:14

This was referenced May 14, 2018

SSO discovery revisited #1080

Merged

Add endpoint to get SAML providers for a user. openedx/edx-platform#18237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jcdyer commented Apr 10, 2018 •

edited by bradenmacdonald

Loading

jcdyer commented Apr 11, 2018

jcdyer commented Apr 12, 2018

xitij2000 left a comment

xitij2000 Apr 11, 2018

xitij2000 Apr 19, 2018

xitij2000 Apr 19, 2018

jcdyer commented Apr 19, 2018

jcdyer commented Apr 19, 2018

xitij2000 commented Apr 19, 2018

jcdyer commented Apr 20, 2018

xitij2000 commented Apr 20, 2018

jcdyer commented Apr 20, 2018

xitij2000 commented Apr 21, 2018

xitij2000 commented Apr 21, 2018

jcdyer commented Apr 21, 2018

xitij2000 commented Apr 21, 2018

xitij2000 left a comment

xitij2000 Apr 21, 2018

jcdyer commented Apr 23, 2018

xitij2000 commented Apr 23, 2018

jcdyer commented Apr 25, 2018

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

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

Conversation

jcdyer commented Apr 10, 2018 • edited by bradenmacdonald Loading

jcdyer commented Apr 11, 2018

jcdyer commented Apr 12, 2018

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 Apr 11, 2018

Choose a reason for hiding this comment

xitij2000 Apr 19, 2018

Choose a reason for hiding this comment

xitij2000 Apr 19, 2018

Choose a reason for hiding this comment

jcdyer commented Apr 19, 2018

jcdyer commented Apr 19, 2018

xitij2000 commented Apr 19, 2018

jcdyer commented Apr 20, 2018

xitij2000 commented Apr 20, 2018

jcdyer commented Apr 20, 2018

xitij2000 commented Apr 21, 2018

xitij2000 commented Apr 21, 2018

jcdyer commented Apr 21, 2018

xitij2000 commented Apr 21, 2018

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 Apr 21, 2018

Choose a reason for hiding this comment

jcdyer commented Apr 23, 2018

xitij2000 commented Apr 23, 2018

jcdyer commented Apr 25, 2018

jcdyer commented Apr 10, 2018 •

edited by bradenmacdonald

Loading