Fix external_auth to properly use retfun for @ssl_login_shortcut()

[ichuang]

The @ssl_login_shortcut() decorator has been broken in the edX codebase for some time, because it did not properly wrap the function being decorated and pass this along to be called after authentication is completed.

This PR fixes that bug.

The changes made have been in production in the residential MITx systems at MIT since spring 2013.

[chrisndodge]

@ormsbee who do you think is best to look at this one?

[chrisndodge]

@ormsbee I'm just going through some of the outstanding pull requests. I'm not too familiar with this area of the code. Can you recommend someone?

[chrisndodge]

@brianhw are you free for a code review? I believe you are familiar with this area of the code-base

[brianhw]

Retfun should be included here as well, for completeness.

[ichuang]

yep, good catch

[brianhw]

This should also check that AUTH_USE_MIT_CERTIFICATES is set. And it should not need a default value of ''.

[ichuang]

the .get() method automatically checks if the key is available, and gives the default if not.

[brianhw]

I don't know if this is as much a question for @ichuang or for @jbau , but it seems there are widely disparate ways of accomplishing the same thing between the Shibboleth and the MIT cert workflows. If I understand the code (and that is quite an 'if'), the shibboleth case is handled in the _external_login_or_signup code, where it ignores the external map's internal password and creates a fake authentication. The cert case is handled by using the internal password in a normal authenticate call, but moves the special logic for linking the external user to the internal user inside the _signup method, which in turn packages up the external map information in the session so it's available to create_account. I don't know which way is better, but it adds to the complexity of the code to have them both if they're effectively accomplishing the same thing. It would therefore help to know whether they have to be different. If so, we should really get that documented in the code somehow.

[brianhw]

It would be good to add a comment here explaining why the import is moved down here. (Because it's a dependency on LMS? Some other reason?)

[brianhw]

And why only this, and not also ModelDataCache?

[ichuang]

done - see comment for reason

[brianhw]

It would also be helpful, in order to protect against the @ssl_login_shortcut() decorator becoming broken in the edX codebase again, to have some tests that confirm that the functionality is indeed working.

[jbau]

hi @brianhw actually, both the shib and the cert flows use _external_login_or_signup as the central dispatcher (I think mainly because for both of these, one can't distinguish the login versus reg. flows based on URL alone.) I think both do very similar things for the registration case (as you said, they first create the external auth object and then package that up in a session variable until an internal user is created, and then they link the two). I added some handling for missing email or full name in the shibboleth cases (by asking for it from the user), but the flow remains basically the same between the two in registration.

For signin, the actual difference is in two places:

1. Shibboleth will try to link against an already-existent internal account before it tries signup
2. Shibboleth doesn't use the password for login, rather it just trusts the external auth and simply logs the user in. This is exactly what the client cert method does, but it just stores the password for the internal account in the external_auth table and uses it. The only drawback we saw for doing this is that users might actually stumble into change their account password, which would break the external_auth login.

If I can be a bit presumptive, I'm guessing the client-cert method can change to use 2. @ichuang what do you think?
Might you also consider just using the codepath of 1. as well?

[ichuang]

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.

But this PR is a bugfix, and changing the code logic is outside its scope.

[chrisndodge]

Echoing @brianhw comments. Any chance for a test here?

[brianhw]

I'm a little confused about the boundary between bugfix and feature here. I understand the bugfix for adding retfun logic. But the addition of the AUTH_USE_MIT_CERTIFICATES_IMMEDIATE_SIGNUP clause doesn't look like a bugfix but the addition of new functionality that wasn't present before. And the way that it's implemented is in a different direction from the acknowledged way the code should be changed. So I'm reluctant to add the new IMMEDIATE_SIGNUP logic. And even moreso without tests.

The advantage to implementing the immediate-signup logic for SSL to be similar to the shib code is that there are lots of examples of tests. And having those tests in place allow for the code to be developed going forward with less fear that a needed feature will remain broken for six months. But I would do it in a separate PR from this one.

[singingwolfboy]

What's the status of this pull request? It needs a rebase; does it need further development before it can be merged, and if so, what's the likelihood that it will actually happen? Would it be simpler to just close this PR, and maybe reopen it later?

[ichuang]

rebased. It doesn't need further development; the PR is in active use at MIT, and has been for several months.

[chrisndodge]

@brianhw @jbau can you eyeball this again. We're trying to get some of the older PR's resolved. Thanks.

[brianhw]

Jason specifically removed this line in an earlier bugfix needed for Shibboleth. Does this have to be added back in?

[ichuang]

no - not needed; removed.

[brianhw]

Not sure what the status of this is. PR 1182 implemented the auto-signup slightly differently from this, so this would need to be rebased and have that change in logic resolved. @carsongee do you have insight?

[carsongee]

I'll take a look at this, rebase, go through the comments, and probably submit a new PR and reference this one if that works for everyone. I would like to get rid of the external auth map password too, as that will allow me to remove a feature from our sysadmin dashboard PR that repairs the table for when they get out of sync.

[carsongee]

@ichuang @brianhw I updated this and submitted it as a new PR #1862 since I don't have permission to rebase this branch. I limited it to this fix and added tests. I'm planning another one for removing dependance on the external password, and kept this one narrowly scoped.

[sarina]

Since @carsongee 's PR #1862 supplants this PR I'm going to close it.