SSO discovery revisited #1080

jcdyer · 2018-05-14T17:48:50Z

Make SSO provider discovery use the same endpoint as the existing request. Gate unprivileged rquests with a setting. Revises work done in #1047.

JIRA tickets: Fixes OC-4285

Discussions: See OC-4285

Dependencies: None

Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: ?

Testing instructions:

Set up a user (username1) with TPA providers.
Enable the ALLOW_UNPRIVILEGED_SSO_PROVIDER_QUERY setting in lms.env.json.
Log in as some other user (username2)
Request /api/third_party_auth/v0/users/username1
See that the providers were returned, but that remote_id was excluded from the response.

Author notes and concerns:

This removes the ability to query by email, since that wasn't part of the old endpoint. It could be added back.
It also changes the response format, to match the existing endpoint.

Reviewers

@xitij2000

FYI: @bradenmacdonald

Settings

ALLOW_UNPRIVILEGED_SSO_PROVIDER_QUERY: true

xitij2000 · 2018-05-15T11:21:27Z

common/djangoapps/third_party_auth/api/views.py

+        """
+        Only throttle unauthenticated requests.
+        """
+        if view.is_unprivileged_query(request, view.kwargs[u'username']):


Maybe the throttle should be in place to privileged queries as well? The issue I see right now is that a someone can just create a regular user account and get past the throttle. So this should either check for staff users or throttle all queries

I didn't want to mess to much with existing queries. The current definition of an "unprivileged" query is one where a non-staff user is requesting information about someone else, or an unauthenticated user is requesting any information. The primary purpose of the throttle is to keep people from using this endpoint to scrape information about a broad number of users. The scenario you present would allow the user to request information about themselves all day long, but they would still be throttled trying to get everyone else's information. I think a general-purpose throttle on api usage is not what we need here, and would be better implemented as part of a general site policy.

Fair enough! I think I misunderstood something here. Your approach seems entirely reasonable.

xitij2000 · 2018-05-15T11:30:49Z

lms/envs/aws.py

@@ -668,7 +668,7 @@
 ORA2_FILE_PREFIX = ENV_TOKENS.get("ORA2_FILE_PREFIX", ORA2_FILE_PREFIX)

 ##### ACCOUNT LOCKOUT DEFAULT PARAMETERS #####
-MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED", 5)
+MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.https://www.google.com/l?q=logging+service&oq=logging+service&aqs=chrome..69i57j0l5.7484j1j4&sourceid=chrome&ie=UTF-8get("MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED", 5)


This seems accidental

Yep. My middle mouse button does double duty as paster and trackpoint scroll, so sometimes it pastes things when I'm trying to scroll. Glad it was just a google search. :D

xitij2000 · 2018-05-15T17:23:02Z

common/djangoapps/third_party_auth/api/views.py

+        """
+        Return True if a non-superuser requests information about another user.
+        """
+        if request.user.username != username:


This will also need adjustment to check if the request user email matches the requested email.

jcdyer · 2018-05-17T20:31:25Z

@xitij2000 This is ready for another review.

xitij2000

👍 Once the issues are addressed.

xitij2000 · 2018-05-17T21:59:36Z

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

-            with patch.object(ThirdPartyAuthProviderApiPermission, 'has_permission', return_value=token_permission):
-                response = self.client.get(url)
-                self.assertEqual(response.status_code, expect)
-


Have you removed these tests on purpose? Is that needed?

xitij2000 · 2018-05-17T22:02:01Z

common/djangoapps/third_party_auth/api/views.py

@@ -142,7 +142,9 @@ def is_unprivileged_query(self, request, username):
        """
        Return True if a non-superuser requests information about another user.
        """
-        if request.user.username != username:
+        # AnonymousUser does not have an email attribute, so fall back to something
+        # that will never comapre equal to username.


nit: comapre -> compare

xitij2000 · 2018-05-17T22:06:49Z

common/djangoapps/third_party_auth/api/urls.py



 PROVIDER_PATTERN = r'(?P<provider_id>[\w.+-]+)(?:\:(?P<idp_slug>[\w.+-]+))?'

 urlpatterns = patterns(
    '',
-    url(r'^v0/users/(?P<identifier>[^/]+)/providers/$', ProviderView.as_view(), name='third_party_auth_providers_api'),
    url(
        r'^v0/users/{username_pattern}$'.format(username_pattern=settings.USERNAME_PATTERN),


Will need to support email pattern here as well.

I think this is covered by the existing pattern. Note that @, ., and + are all included:

From lms/envs/common.py

USERNAME_REGEX_PARTIAL = r'[\w .@_+-]+' USERNAME_PATTERN = r'(?P<username>{regex})'.format(regex=USERNAME_REGEX_PARTIAL)

Hmm. I thought it was configurable, but it doesn't seem to be, so I guess it's OK.

jcdyer · 2018-05-20T14:37:57Z

Issues addressed. Tests are running again.

xitij2000 · 2018-05-20T21:27:36Z

@jcdyer I think there is just one very minor PEP8 fix left.

OC-4285

jcdyer · 2018-05-21T00:48:17Z

Fixed and squashed. Will merge after CI finishes.

openedx-webhooks added needs triage open-source-contribution labels May 14, 2018

edx deleted a comment from openedx-webhooks May 14, 2018

mduboseedx removed needs triage open-source-contribution labels May 14, 2018

jcdyer mentioned this pull request May 15, 2018

SSO discovery revisited [WIP] openedx/edx-platform#18192

Closed

xitij2000 suggested changes May 15, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-revisited branch from f1bc614 to 58a65fc Compare May 16, 2018 21:10

xitij2000 suggested changes May 17, 2018

View reviewed changes

jcdyer force-pushed the cliff/sso-discovery-revisited branch from abd7b54 to 1e7e7f0 Compare May 20, 2018 17:48

jcdyer mentioned this pull request May 20, 2018

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

Merged

2 tasks

jcdyer force-pushed the cliff/sso-discovery-revisited branch from a43daf8 to 1e7e7f0 Compare May 20, 2018 17:57

xitij2000 approved these changes May 20, 2018

View reviewed changes

Combine user/provider SSO views.

90f0394

OC-4285

jcdyer force-pushed the cliff/sso-discovery-revisited branch from 1e7e7f0 to 90f0394 Compare May 21, 2018 00:46

jcdyer merged commit 5dc4c93 into integration May 21, 2018

xitij2000 mentioned this pull request May 23, 2018

[MCKIN-7315] SSO provider endpoint reworked #1088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSO discovery revisited #1080

SSO discovery revisited #1080

jcdyer commented May 14, 2018

xitij2000 May 15, 2018

jcdyer May 15, 2018

xitij2000 May 17, 2018

xitij2000 May 15, 2018

jcdyer May 15, 2018

xitij2000 May 15, 2018

jcdyer commented May 17, 2018

xitij2000 left a comment

xitij2000 May 17, 2018

xitij2000 May 17, 2018

jcdyer May 20, 2018

xitij2000 May 17, 2018

jcdyer May 20, 2018

xitij2000 May 20, 2018

jcdyer commented May 20, 2018

xitij2000 commented May 20, 2018

jcdyer commented May 21, 2018

SSO discovery revisited #1080

SSO discovery revisited #1080

Conversation

jcdyer commented May 14, 2018

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 May 17, 2018

xitij2000 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

Choose a reason for hiding this comment

jcdyer commented May 20, 2018

xitij2000 commented May 20, 2018

jcdyer commented May 21, 2018