ENT-669: Activation email received when skip_email_verification flag is 'True'

[saleem-latif]

Description:
A SuccessFactors customer recently reported that one of their testers received an activation email message even though the SAML Provider Config profile's skip_email_verification flag is set to True.

Because this is a SuccessFactors integration, the initial authentication is indeed happening via SAML, however the email address is collected at a later point using a callback to the SuccessFactors OData API, which is why the email correctly appears on the user's edX account but it was not present in the original pipeline data.

[douglashall]

I think we want to leave the email check in there if this is not an IdP that makes use of the OData API. Perhaps we could check the type of IdP and ignore the email check if we are dealing with a SF IdP. Something like the following?

not (
    third_party_provider and third_party_provider.skip_email_verification and
    (
        user.email == running_pipeline['kwargs'].get('details', {}).get('email') or
        third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY
    )
)

[saleem-latif]

Done. I have also refactored code a bit to make it less confusing and added tests for the changes.

[douglashall]

I love the refactor here! Let's just tighten it up a bit more. Thanks.

[douglashall]

Name this skip_activation_email.

[douglashall]

sso_pipeline_email = running_pipeline and running_pipeline['kwargs'].get('details', {}).get('email')

# Email is valid if the SAML assertion email matches the user account email or
# no email was provided in the SAML assertion. Some IdP's use a callback
# to retrieve additional user account information (including email) after the
# initial account creation.
valid_email = sso_pipeline_email == user.email or sso_pipeline_email is None

return (
    settings.FEATURES.get('SKIP_EMAIL_VALIDATION', None) or
    settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING') or
    (settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH') and do_external_auth) or
    (third_party_provider and third_party_provider.skip_email_verification and valid_email)

[douglashall]

The valid email check should actually be:

from common.djangoapps.third_party_auth.saml import SAP_SUCCESSFACTORS_SAML_KEY

valid_email = (
    sso_pipeline_email == user.email or (
        sso_pipeline_email is None and
        third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY
    )
)

[saleem-latif]

third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY check does not for SAML IdP that are not associated with SAP.
With above check activation email is being sent to when third_party_provider.skip_email_verification == True and I am signing in using testship IdP.
here is the sample details dict in the IdP response.

{
    u'username': u'myself', 
    u'fullname': u'Me Myself And I', 
    u'last_name': u'And I', 
    u'first_name': u'Me Myself', 
    u'email': None
}

[douglashall]

Sorry, I'm not following your last comment regarding the identity_provider_type check. I think we only want to skip the activation email when the SAML email address is None AND the IdP is SuccessFactors (or some other IdP that uses some kind of callback to get additional user profile information).

[saleem-latif]

I made the changes you request above

valid_email = (
   sso_pipeline_email == user.email or (
       sso_pipeline_email is None and
       third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY
   )
)

but it was not skipping email (for testshib IdP) for the following reason

1. sso_pipeline_email is None returned True but third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY returned False.

second condition failed because third_party_provider.identity_provider_type was standard_saml_provider instead of sap_success_factors.

So, I am guessing that there might be identity providers for which third_party_provider.identity_provider_type == SAP_SUCCESSFACTORS_SAML_KEY would fail and they would have missing email in user data, hence we should remove the above condition from the expression.

[douglashall]

That’s the point. We would not want to skip sending the activation email under the condition you describe in #1 in your last comment.

[saleem-latif]

Done

[douglashall]

Can we just return the value of the statement above instead of assigning to the skip_email variable?

[saleem-latif]

Done, Sorry I missed this before

[douglashall]

This looks good to go once the tests are passing.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, October 31, 2017.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.