flipboxfactory / saml-sp

SAML Service Provider (SP) Plugin for Craft CMS
https://saml-sp.flipboxfactory.com/
Other
19 stars 5 forks source link

User save failed: Email has already been taken. #91

Closed paulbrough closed 3 years ago

paulbrough commented 3 years ago

Hello, we've had a couple of users who were able to log in via SAML repetedly, but then get this error: User save failed: {"email":["Email \"person@example.com\" has already been taken."]}

I'm not sure if it's relevant, but we do have this in our general Craft config: 'useEmailAsUsername' => true

Thanks for your help!

dsmrt commented 3 years ago

Hey @paulbrough 👋 ,

Do you have any configuration overrides in the saml-sp or yii2 event hooks (Event::on) that might be modifying that user on or before save?

Also, version of the plugin are you on?

paulbrough commented 3 years ago

Hi @dsmrt

Here's the saml-sp config:

return [
    'groupAttributeNames' => ['http://schemas.xmlsoap.org/claims/Group']
];

The only thing user-related event hook I can think of is registering a new user permission (probably unrelated, but including in case it means anything):

Event::on(
    UserPermissions::class,
    UserPermissions::EVENT_REGISTER_PERMISSIONS,
    function(RegisterUserPermissionsEvent $event) {
        $event->permissions['Content Viewing'] = [
            'viewAllContent' => [
                'label' => 'View All Content',
            ],
        ];
    }
);

Plugin version is 2.5.1

dsmrt commented 3 years ago

Ok, nothing that stands out with that.

The question is, why wouldn't this find the user? Anything on the specific user that'd help explain the error?

https://github.com/flipboxfactory/saml-sp/blob/824b7f239240b4fb89e28532fddec75e216b664a/src/services/login/User.php#L413-L427

One other thing that I can think of is if there's a nameIdOverride that is confusing things? See: https://github.com/flipboxfactory/saml-sp/blob/824b7f239240b4fb89e28532fddec75e216b664a/src/services/login/User.php#L49-L85

paulbrough commented 3 years ago

I think I tracked down the issue: SAML users have usernames that are UID strings like 7e3d2nc4-14aa-4441-b0cb-b836657326re (from the <NameID> element of the SAML assertion). The problematic users have email addresses for usernames.

Turns out that when the useEmailAsUsername config setting is true, any user edits (in the CP or front-end forms) will change the username field to the user's email. On the next login attempt, there's no longer a matching existing user, so it tries to create a new one, but can't because the email is already taken.

Thanks so much for the quick responses!

dsmrt commented 3 years ago

Good catch! That makes sense.