SSO - Ability to specify default third_party_auth provider via query param by bradenmacdonald · Pull Request #8591 · openedx/openedx-platform

bradenmacdonald · 2015-06-19T18:29:18Z

Description:
If link to a course includes a query parameter (?tpa_hint=) that specifies one of the enabled third_party_auth providers, and the user is not logged in, the third_party_auth sign in flow with the specified provider should be automatically started instead of redirecting the user to the login page.

Background info in the SSO MVP Spec Doc

JIRA: OSPR-664

No un-merged dependencies.

Screenshot:

Sandbox:
http://sandbox5.opencraft.com/ (Updated Jun 25)

Testing Instructions (Sandbox):
Here is a link to the courseware:
http://sandbox5.opencraft.com/courses/edX/DemoX/Demo_Course/courseware/d8a6192ade314473a78242dfeedfbf5b/edx_introduction/

Here is a hinted link to the courseware, using the UBC SAML provider:
http://sandbox5.opencraft.com/courses/edX/DemoX/Demo_Course/courseware/d8a6192ade314473a78242dfeedfbf5b/edx_introduction/?tpa_hint=saml-ubc-staging

Here is a hinted link to the courseware, using the Dummy provider:
http://sandbox5.opencraft.com/courses/edX/DemoX/Demo_Course/courseware/d8a6192ade314473a78242dfeedfbf5b/edx_introduction/?tpa_hint=oa2-dummy

Click on all three links. Note the different behaviour. (Assuming you are not currently logged in to our sandbox)
Open the Dummy-hinted version in an incognito window, and use sign in with the Dummy provider. You should now be logged in and taken to the courseware.
Log out of Adama's account.
Open the UBC-hinted version in an incognito window, but instead of signing in via UBC, try signing in as student@example.com or using the Dummy provider. You should be logged in and taken to the courseware.

Testing Instructions (Local):

Create a course and enable at least one third party auth provider (e.g. add a Dummy provider using the dummy backend at localhost:8000/admin/third_party_auth/oauth2providerconfig/ ).
Re-create the scenario above but using your local course.

Reviewers:
All: @Kelketek and Cale?
Product: Beth and/or Leslie (asked via email)
UX: @talbs
a11y: @cptvitamin

openedx-webhooks · 2015-06-19T18:29:18Z

Thanks for the pull request, @bradenmacdonald! I've created OSPR-664 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.

bradenmacdonald · 2015-06-19T21:30:25Z

Jenkins, test this please. flaky test failure.

Kelketek · 2015-06-19T23:11:10Z

Nit: Weird spacing here.

True; I had just copy-pasted that line from account_spec. If I need to amend this commit I'll fix that up :)

Kelketek · 2015-06-19T23:15:29Z

@bradenmacdonald This looks solid to me. 👍

bradenmacdonald · 2015-06-19T23:38:55Z

@cpennington @sarina This PR is ready for review. It targets the SSO feature branch and is mostly JavaScript and HTML changes. Let me know if you want an OSPR.

cpennington · 2015-06-22T16:13:46Z

Question: does it make sense to have this as a query param, rather than as a separate url?

My thought was that a query param is easier for partner universities to construct - just copy the URL to some part of the courseware, then append the query string part. If it's a separate URL we'd probably need to create a UI/tool that creates the URL for them, no? And there may be many different types of links that we'd need to accommodate - so several new URL patterns may be required.

Edit: I guess something like /hinted_login?next=... would work as well, as a new URL. It would just be harder for partner universities to construct such a URL. It is likely a cleaner and more generic approach though.

I that makes sense. I was thinking there was just one url, but if the idea is to just have any of our normal urls, but decorate with an optional query param, then that sounds fine.

sarina · 2015-06-23T14:31:59Z

@cpennington should the platform team or destination team review this?

cpennington · 2015-06-23T17:01:31Z

Is there binding that I'm missing that makes this button work?

@cpennington Yep, it's an existing handler for the page as a whole in AccessView: https://github.com/edx/edx-platform/pull/8591/files#diff-aa10400ca417043aab41f33394c90790R15

cpennington · 2015-06-23T17:01:56Z

👍 from a high level review.

sarina · 2015-06-23T17:06:44Z

@cpennington does someone else need to review this as well?

sarina · 2015-06-23T17:34:27Z

Just a note (totally don't have to fix this) but in python since you can define a string with single or double quotes, if you define it with single quotes you don't have to escape the double quotes (and vice versa).

sarina · 2015-06-23T17:37:48Z

@mhoeber please assess for doc impact

cpennington · 2015-06-23T18:08:09Z

@sarina, no, it's good (assuming we decide there's no UX or doc impact)

sarina · 2015-06-23T18:26:41Z

Docs are going to come after this merges, so we are not blocked on docs. Just waiting on UX, which should happen this week. Thanks!

talbs · 2015-06-25T21:41:01Z

You're really compounding this selector (especially with https://github.com/edx/edx-platform/pull/8603). Please place each new selector on a different line for better legibility with that in mind.

When adding structural or styling-minded wrapper elements, we add the wrapper- prefix. Please change to wrapper-hint.

talbs · 2015-06-25T21:46:40Z

Visual Feedback: You've got some uneven spacing between your auth buttons (screengrab below). Please make this consistent.

talbs · 2015-06-25T21:48:40Z

When adding structural or styling-minded wrapper elements, we add the wrapper- prefix. Please change to wrapper-hint.

Any objection to me changing this to wrapper-other-login, the same class that the other PR is using for the same purpose?

talbs · 2015-06-25T21:49:33Z

@bradenmacdonald, thanks for the work on this. Its much appreciated.
I've left my Front End (HTML, Sass/CSS) feedback as well as some visual notes to address.

Assuming that this is just being released on Edge and that @ebporter has reviewed from a Product perspective, this is fine from a UX perspective. Let me know once you've addressed my feedback and if you have any questions.

bradenmacdonald · 2015-06-25T21:59:41Z

@talbs re: "uneven spacing between your auth buttons (screengrab below). Please make this consistent." - that was an existing issue and I've fixed it with the other PR. When both are merged, it will be fixed :)

bradenmacdonald · 2015-06-26T02:08:19Z

@talbs Ok, I just pushed a commit to fix:

serif font was being used
hint-wrapper class replaced with same wrapper-other-login class as the other PR, since it plays the same role

I replied to all the other comments and updated the sandbox.

talbs · 2015-06-26T02:34:00Z

Thanks, @bradenmacdonald. The fixes and sandbox look generally good. I left one visual thing (in what looks like a collapsed thread in this PR's history) about a display issue with the default provider button state.

Once you've addressed that, 👍

bradenmacdonald · 2015-06-26T02:40:36Z

@cptvitamin Can you please review this for a11y? It's just the one fairly minimal new page shown in the screenshot in the PR description above, and linked to under "Testing Instructions".

Like the other PR where Brian just pinged you, this is extending the existing logistration page and we're trying to get merged in prior to the next release.

talbs · 2015-06-26T02:44:00Z

@bradenmacdonald, thanks for the extra clarification.

👍

ebporter · 2015-06-26T18:07:03Z

Looks good from a Product perspective.

cptvitamin · 2015-06-26T18:25:12Z

👍 This looks great

sarina · 2015-06-26T18:45:07Z

Braden - go ahead and merge this one!

On Fri, Jun 26, 2015 at 2:25 PM, Mark Sadecki notifications@github.com
wrote:

[image: 👍] This looks great

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/8591#issuecomment-115824267.

bradenmacdonald · 2015-06-26T18:55:13Z

Great, thanks @cptvitamin and everyone :)

This one needs a rebase now that the other has merged and I just noticed a bok choy failure that's showing up now, likely from renaming CSS classes during the UX review. I'll rebase, fix that test, and merge as soon as the tests pass (save that unrelated test failure that's fixed on master).

bradenmacdonald mentioned this pull request Jun 19, 2015

Feature: Shibboleth/SAML SSO #8140

Merged

14 tasks

bradenmacdonald force-pushed the feature/shibboleth-tpa branch from 3bccf35 to c6cf374 Compare June 19, 2015 19:39

bradenmacdonald force-pushed the shibboleth-3 branch 2 times, most recently from 855ed9b to 6eefc63 Compare June 19, 2015 20:53

Kelketek reviewed Jun 19, 2015
View reviewed changes

cpennington reviewed Jun 22, 2015
View reviewed changes

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Jun 22, 2015

cpennington reviewed Jun 23, 2015
View reviewed changes

sarina added awaiting prioritization and removed needs triage labels Jun 23, 2015

sarina reviewed Jun 23, 2015
View reviewed changes

talbs reviewed Jun 25, 2015
View reviewed changes

openedx-webhooks added engineering review and removed awaiting prioritization labels Jun 26, 2015

bradenmacdonald added 4 commits June 26, 2015 12:11

New Hinted Login View

f42628c

Tests

8360a72

front end fixes per UX review

9d490c9

Fix bok choy test

8362058

bradenmacdonald force-pushed the shibboleth-3 branch from 78206a8 to 8362058 Compare June 26, 2015 19:27

bradenmacdonald merged this pull request into openedx:feature/shibboleth-tpa Jun 26, 2015

bradenmacdonald deleted the shibboleth-3 branch June 26, 2015 20:15

bradenmacdonald mentioned this pull request Jul 7, 2015

Fix hinted login view to be compatible with secondary providers. #8815

Merged

bradenmacdonald mentioned this pull request Jul 22, 2015

Ahodges/sso open openedx-unsupported/edx-documentation#452

Merged

Uh oh!

Conversation

bradenmacdonald commented Jun 19, 2015

Uh oh!

openedx-webhooks commented Jun 19, 2015

Uh oh!

bradenmacdonald commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kelketek commented Jun 19, 2015

Uh oh!

bradenmacdonald commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarina commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpennington commented Jun 23, 2015

Uh oh!

sarina commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarina commented Jun 23, 2015

Uh oh!

cpennington commented Jun 23, 2015

Uh oh!

sarina commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talbs commented Jun 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talbs commented Jun 25, 2015

Uh oh!

bradenmacdonald commented Jun 25, 2015

Uh oh!

bradenmacdonald commented Jun 26, 2015

Uh oh!

talbs commented Jun 26, 2015

Uh oh!

bradenmacdonald commented Jun 26, 2015

Uh oh!

talbs commented Jun 26, 2015

Uh oh!

ebporter commented Jun 26, 2015

Uh oh!

cptvitamin commented Jun 26, 2015

Uh oh!

sarina commented Jun 26, 2015

Uh oh!

bradenmacdonald commented Jun 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants