New third party provider configuration options

[bradenmacdonald]

This implements three new options for third party auth providers. All three options are controlled via the Django admin (ConfigurationModel[s]).

1. Secondary: Secondary providers are displayed less prominently, in a separate list of "Institution" login providers
2. Skip Registration Form: If this option is enabled, users will not be asked to confirm their details (name, email, etc.) during the registration process. Only select this option for trusted providers that are known to provide accurate user information.
3. Skip Email Verification: If this option is selected, users will not be required to confirm their email, and their account will be activated immediately upon registration.

Screenshots:
Look for the new "Use Institution Credentials" button in these screenshots:

And here's what would be seen when one clicks on the link (except it would be an alphabetical list of real universities):

Sandbox:
http://sandbox4.opencraft.com/login (Updated June 25 to 5f6cbab)

Reviewers:
Engineering: @Kelketek and @cpennington
Product: @ebporter
UX: @talbs
a11y: @cptvitamin

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?number=8603&repo=edx%2Fedx-platform

[openedx-webhooks]

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

[Kelketek]

@bradenmacdonald You've got JS test failures which appear to be related to this task.

[Kelketek]

I believe the UX team is trying to get away from specific pixel values like this in favor of predefined variables that handle spacing. I'm not sure if it will be requested here or not, since nothing else in the file is so updated, so maybe they'd rather handle it in a future task. Just a heads up. I'm sure @talbs or someone else on that team may clarify if it's needed.

[talbs]

@Kelketek is correct, for anything that can be a derivative of our $baseline Sass spacing/layout scale variable (~20px), please reference it. In this case, something like ($baseline/4) would be best.

[talbs]

Also, add RTL support.

[Kelketek]

@bradenmacdonald The sandbox makes it difficult to see if the tertiary providers can actually be used, as all of them appear to be disabled.

[bradenmacdonald]

@Kelketek Yeah, I'll look into the JS failures. For some reason they're passing locally so I was having trouble figuring out what the cause it.

[Kelketek]

@bradenmacdonald I know they just recently did some sort of RequireJS optimizations that caused these items to be pipelined, and I think they're doing some stuff to make it fail early if your assets aren't put in the build pipeline, but I'm not sure.

https://openedx.atlassian.net/browse/SOL-977 The earlier issues with Content Libraries were solved by adding the JS assets to the required pipeline, so maybe that's something you need to do if you added some assets and haven't plugged them in.

[bradenmacdonald]

FYI, there is a "TestEmailMessageWithCustomICRVBlock" test failure that is unrelated. It is what I call a "test time bomb" - a hard-coded date that causes the test to fail after June 22, 2015.

[bradenmacdonald]

@Kelketek I have added two "TestShib" provider options to the sandbox, and they should now be active. The rest of the University login options are not enabled, as you noticed. They're just there to show the UI.

[Kelketek]

@bradenmacdonald Looks good to me. 👍 if the tests pass.

[bradenmacdonald]

@cpennington This is ready for your review.

"TestEmailMessageWithCustomICRVBlock" test failure is unrelated.

[sarina]

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

[cpennington]

👍

[sarina]

👍

@bradenmacdonald rebasing should pick up test fixes.

@talbs still needs to review. @mhoeber please assess for doc impact.

[talbs]

Do you need to specify a height here? If not, I'd prefer this removed and the element's height be automatically calculated from its contents. If you need to sync up heights across buttons, I'd recommend either min-height.

[bradenmacdonald]

This is also not something I changed. I can convert this to min-height if you want, but then you get this (sorry this is in RTL mode, but it still illustrates the point):

Currently the buttons don't expand at all, and extra text gets cut off. An alternative is to change the width to min-width, which I think is less bad:

That said, these styles are not part of my change so let me know if we can postpone this.

[talbs]

Gotcha. Apologies. Yeah, we can definitely table this and make it part of edX's clean up.

[talbs]

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

[bradenmacdonald]

@talbs Thanks for the review - I appreciate you fitting it in to this week!

I have addressed all your comments (I think!):

• Various SASS cleanups, e.g. more use of $baseline, re-ordering rules, fewer hard-coded values. (n.b. a lot of your comments were on existing lines that I didn't change. There are still quite a number of issues remaining in that .scss file like non-standard typography, em units, etc. but I only fixed the ones that I changed plus the ones that you specifically asked me to do as "drive by" fixes.)
• Tested everything with RTL (The Google/Facebook/etc buttons previously didn't work but now they do)
• Fixed the issue where if you went Login > Institution Login > Login > Register > Institution Register, the back button was wrong
• Fixed the hover state issue you mentioned
• Updated the sandbox

You can see my commits above if you'd like to review the diff.

[talbs]

Wow, thanks for the quick turn around, @bradenmacdonald. Things look great all around! I'll circle back with edX Product on some other points, but this is solid from a UX PoV.

👍

[talbs]

Oh, also, @bradenmacdonald. Apologies for the feedback on existing code. I partook in far too much coffee today. That always means me on a Sass/CSS cleaning frenzy. :)

[bradenmacdonald]

:-)

Thanks @talbs. Cheers.

[sarina]

shit I did forget to tag @cptvitamin on these. Braden can you tag him on
the other PR? Mark for context this is extending the existing logistration
page and we're trying to get merged in prior to the nextrelease.

On Thu, Jun 25, 2015 at 10:30 PM, Braden MacDonald <notifications@github.com

wrote:

:-)

Thanks @talbs https://github.com/talbs. Cheers.

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

[ebporter]

This looks good from a product perspective. @talbs I know that edge reg/enroll flows need work. We can tackle them in our Onboarding UX Audit. So, add edge to the list (after Open & White Label). We'll want to pull this SSO flow onto edX.org at some point in the not too distant future; so, that's when we can tackle the outstanding UX issues.

[cptvitamin]

@bradenmacdonald I have a little bit of feedback here from an accessibility perspective. None of them are show-stoppers. If you wanted to introduce them in another PR, that would be cool:

1. From a visual perspective, its easy to figure out what my options are just by glancing at the page. But if I go through the page linearly, from a screen reader user's perspective, I wouldn't know that anything existed beyond the Sign-in Button. For situations like these, its nice to provide some instructions at the top of the form that explain what options are. They can be displayed offscreen with class="sr". It may be tempting to link to the parts of the form from the instructions, but offscreen text should never contain interactive elements since that would be confusing to sighted keyboard only users (the focus disappears for a bit while cycling through these elements offscreen.)
2. (this is an existing issue I imagine, but if we can fix it here, lets do it) The Dummy, Google, Facebook buttons do not make sense out of context. If I am a screen reader user and I decide I'm going to navigate/interact with this page by looking at my form fields list box (this is a typical interaction pattern for Viz users) I see buttons that say Dummy, Google, Facebook. I would assume that I those buttons would take me to those sites, or perhaps +1 edX or the course on Google, or like it on Facebook. I wouldn't necessarily assume without any additional context that I would be logging in. Most interaction patterns (form list dialog, or tabbing from focusable element to focusable element) will skip right past the "or sign in with" text. I suggest adding offscreen (sr) text before each button text that says "Login with "

That's it! Otherwise, this looks great.

[sarina]

Braden, I just spoke with Mark. Let's merge this now to get into the
release branch, and please prepare a PR and sandbox with these fixes by
Monday so we can make sure they'll make it into Cypress.

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

@bradenmacdonald https://github.com/bradenmacdonald I have a little bit
of feedback here from an accessibility perspective. None of them are
show-stoppers. If you wanted to introduce them in another PR, that would be
cool:

1. From a visual perspective, its easy to figure out what my options
   are just by glancing at the page. But if I go through the page linearly,
   from a screen reader user's perspective, I wouldn't know that anything
   existed beyond the Sign-in Button. For situations like these, its nice to
   provide some instructions at the top of the form that explain what options
   are. They can be displayed offscreen with class="sr". It may be tempting to
   link to the parts of the form from the instructions, but offscreen text
   should never contain interactive elements since that would be confusing to
   sighted keyboard only users (the focus disappears for a bit while cycling
   through these elements offscreen.)
2. (this is an existing issue I imagine, but if we can fix her here,
   lets do it) The Dummy, Google, Facebook buttons do not make sense out of
   context. If I am a screen reader user and I decide I'm going to
   navigate/interact with this page by looking at my form fields list box
   (this is a typical interaction pattern for Viz users) I see buttons that
   say Dummy, Google, Facebook. I would assume that I those buttons would take
   me to those sites, or perhaps +1 edX or the course on Google, or like it on
   Facebook. I wouldn't necessarily assume without any additional context that
   I would be logging in. Most interaction patterns (form list dialog, or
   tabbing from focusable element to focusable element) will skip right past
   the "or sign in with" text. I suggest adding offscreen (sr) text before
   each button text that says "Login with "

That's it! Otherwise, this looks great.

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

[bradenmacdonald]

@cptvitamin Thanks for the review! I think both of those would be great improvements but they do both sound like existing issues so I agree with the plan of merging this now, and I'll prepare a separate PR for those later today.

[bradenmacdonald]

Ok, follow-up a11y PR posted at https://github.com/edx/edx-platform/pull/8699

[sarina]

@bradenmacdonald the requirement of python-saml is causing localdev to break for all of us who've rebased. Can you investigate and fix ASAP? This is the output I get:

$ pip install -q --exists-action w -r requirements/edx/base.txt
You are using pip version 6.0.8, however version 7.0.3 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
  meliae is potentially insecure and unverifiable.
  Requested meliae==0.4.0 (from -r requirements/edx/base.txt (line 53)), but installing version 0.4.0.final.0
    Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-LbcHbX/dm.xmlsec.binding

Removing python-saml from my requirements file fixes the paver install_prereqs task for me. @andy-armstrong has seen this failure as well and I'm sure as more developers rebase early next week they'll see this too, so this is critical to fix.

[bradenmacdonald]

@sarina To fix that, there are two ubuntu packages that must be installed:

sudo apt-get install libxmlsec1-dev swig

I knew about these dependencies so I put them into requirements/system/ubuntu/apt-packages.txt as part of the recent feature branch merge, and also I had added them to edx-configuration in playbooks/roles/edxapp_common/defaults/main.yml back in May:

• edx-platform commit
• edx-configuration PR

Is there somewhere else that they need to be listed, or do we just have to ask people to manually install them? (Or re-provision?)

[sarina]

@bradenmacdonald It would be good to send an email to eng-global about this change. I was aware of your PRs and didn't put it together.

Please also forward the email to edx-code (ideally after people on eng-global confirm the instructions work for them).

[andy-armstrong]

@bradenmacdonald @sarina This didn't work for me on my devstack. Am I doing this wrong?

vagrant@precise64:~$ sudo apt-get install libxmlsec1-dev swig --fix-missing
sudo apt-get install libxmlsec1-dev swig --fix-missing
Reading package lists... Done
Building dependency tree       
Reading state information... Done
The following extra packages will be installed:
  libnspr4-dev libnss3-dev libxmlsec1 libxmlsec1-gnutls libxmlsec1-nss
  libxmlsec1-openssl swig2.0
Suggested packages:
  swig-doc swig-examples swig2.0-examples swig2.0-doc
The following NEW packages will be installed:
  libnspr4-dev libnss3-dev libxmlsec1 libxmlsec1-dev libxmlsec1-gnutls
  libxmlsec1-nss libxmlsec1-openssl swig swig2.0
0 upgraded, 9 newly installed, 0 to remove and 152 not upgraded.
Need to get 330 kB/2,952 kB of archives.
After this operation, 13.0 MB of additional disk space will be used.
Do you want to continue [Y/n]? Y
Y
Err http://us.archive.ubuntu.com/ubuntu/ precise-updates/main libnss3-dev amd64 3.17.1-0ubuntu0.12.04.1
  404  Not Found [IP: 91.189.91.15 80]
Err http://security.ubuntu.com/ubuntu/ precise-security/main libnss3-dev amd64 3.17.1-0ubuntu0.12.04.1
  404  Not Found [IP: 91.189.91.13 80]
Failed to fetch http://security.ubuntu.com/ubuntu/pool/main/n/nss/libnss3-dev_3.17.1-0ubuntu0.12.04.1_amd64.deb  404  Not Found [IP: 91.189.91.13 80]
Unable to correct missing packages.
E: Aborting install.

[bradenmacdonald]

@andy-armstrong You likely just need to do sudo apt-get update first. Looks like ubuntu has packaged a newer version of libnss3-dev, so the version your system is trying to download is no longer on the mirrors.

[andy-armstrong]

Thanks @bradenmacdonald. That indeed did it for me.