Add feature to do auto signup with external auth

[carsongee]

This adds a feature flag: AUTH_USE_MIT_CERTIFICATES_IMMEDIATE_SIGNUP
that does an automatic signup of users if they are using external authentcation.

[singingwolfboy]

What's going on with this indentation?

[singingwolfboy]

This needs a unit test to cover this feature. You should also add yourself to the AUTHORS file, and sign the individual contributor agreement if you haven't yet.

[carsongee]

Is there a preference as to where I build that test? There doesn't really seem to be a base external auth test class and this is a pretty minor feature that just needs to make sure a user is activated and created. Should I make a test.py? Or should it be something like test_bypass_signup.py ?

[wedaly]

My preference would be to create a new module test_signup.py

[brianhw]

Please see comments on https://github.com/edx/edx-platform/pull/653, which introduces pretty much the same change. In that, Ike agreed with @jbau: "the SSL auth path should migrate to simply log the user in; users do indeed accidentally sometimes stumble into changing their password, breaking the external_auth login, so the Shibboleth approach is better." So I would encourage this to be rewritten to fit into the model set up by the shib code.

I would suggest that the tests for this be put in test_ssl, since the existing tests are in test_shib and test_openid_provider. I would expect that the code in test_shib might serve as a good model for writing ssl tests.

[sarina]

Hi everyone - what's the status of this PR? It still lacks tests and hasn't been active for a month.

[carsongee]

I was planning on adding tests, but it looked like the scope changed based on this getting referenced into PR #653 and the basics of ssl auth needed to get changed at the root before this change was accepted. I haven't had time for that yet. If we can let this small feature go with just some unit tests to exercise this 10 liner, I'd be happy to do this and get this PR finished.

[sarina]

I'll let @brianhw or @jbau make the call as to whether or not this is a feature we want to add to the code base. If we don't want it as-is with tests, then I suggest closing this PR until the feature can be worked on.

[sarina]

@brianhw or @jbau can you please look in on this outstanding PR? See my above comment.

[brianhw]

Interesting negotiation. If you write tests for this feature as it is in such a way that the tests would continue to work for its reimplementation (along the lines of the previous discussion), then I would find the current code more acceptable. I would be even happier if there were also a plan to actually improve the overall code to be more consistent sometime in the near future. But having tests to make that code change would be enough to sell me.

Again, I would suggest something like test_ssl, that mirrors the other tests.

[carsongee]

I've added tests here for the signup. I've also added a couple test that are skipping now that if they passed would allow SSL authentication to work properly in CMS. My plan is to get those tests passing, redo ssl auth handling to make it more similar to shib, and make SSL logins happen more smoothly (e.g. additional code around the DISABLE_LOGIN_BUTTON feature). Throughout that, I plan on adding additional tests in that area of the code related to those refinements (and others I come up with as I go). I can't give a very accurate timeline of this as of now, but I would expect in the next month or so.

[brianhw]

pylint complains: "261: W1201: (logging-not-lazy), _signup: Specify string format arguments as logging function parameters "

[sarina]

This just means change the logging call to read

log.info('doing immediate signup for %s, params=%s', username, post_vars)

[sarina]

To see your reports, on the bottom or top of the page click the "Details" link (looks like this:

On the left side of the page you're brought to will be two reports you need to review: the Diff Coverage report and the Diff Quality report. We expect your testing coverage to be ~95% or better (diff coverage) and your pep8 and pylint quality to each be 100% (diff quality). If you have troubles resolving pylint errors, there are ways to get Pylint to ignore the error by adding a pragma. Ask us for help if you want to know if it's ok to use a pragma.

[brianhw]

As @sarina noted, there are a bunch of pylint errors showing up due to the super call and an assortment of unused variables. Fix those, and I'm okay with this going in. Thanks for writing those tests!

[carsongee]

I have corrected the mentioned issues. Thank you for the quick review. It is worth noting that I also added external_auth to the list of installed apps on CMS so the eamap models would get populated. This allowed the authentication with auto signup test to pass in CMS. I can remove that if we think that is out of scope for this change though as I realize it may have some side effects.

[brianhw]

I would prefer to have the CMS activation in a separate PR. They have a slightly different login workflow that I don't fully understand, and I would prefer someone else to review that addition. Let's just get this one in.

[carsongee]

No problem. I moved that out, and I'll move that to another task (that PR should make both the cms tests work anyway, and probably some other parts).

[brianhw]

Great! Once tests pass, this is good to go. 👍

[sarina]

@brianhw can you merge or make more comments? (edx employees need to merge external contributions)