search

verbb / social-login

A Craft CMS plugin to add SSO (Single Sign-On) user login, registration and connecting to social media platforms.
Other
4 stars 6 forks source link

Syncing first/last name fields on an existing user without a fullName mapping will not update fullName #32

Open Braedan opened 4 weeks ago

Braedan commented 4 weeks ago

Describe the bug

When syncing an existing user profile, if fullName is set and there's no mapping set on the provider (which is the case for Google, and likely others), it can fall out of sync with the firstName and lastName fields.

Craft handles this on User::afterSave via NameTrait::prepareNamesForSave(), but As of 4.9 only if fullName or first/last name is null.

(Sorry in advance if this should be a feature request; its hard to tell what the expected behaviour should be)

Steps to reproduce

  1. Set populateProfile and syncProfile to true in social settings
  2. Create a user that will share an email with an SSO login, set Full Name to fName lName (assuming SSO first/last name isn't the same)
  3. Log in via SSO to any provider that has fieldMapping for firstName and lastName but not fullName (eg; Google provider)
  4. Updated User element will save, but Full Name will not be synced with new first name / last name values

Craft CMS version

4.9.5

Plugin version

1.0.15

Multi-site?

No

Additional context

To fix, could use User::setAttributes() instead of manually assigning values onto the user object in services\Users::_syncUserProfile(). The function handles firstName / lastName logic (see: https://github.com/craftcms/cms/blob/5.x/src/elements/User.php#L995-L1006).

Alternatively, adding an event onto _syncUserProfile where I could manually set fullName = null would be handy, as right now I'm resolving by emulating the setAttributes conditional on User::EVENT_BEFORE_SAVE

engram-design commented 4 weeks ago

I suppose this is true for anything when syncProfile is true - where should be the source of truth? If it's always the offsite provider, then yes we should overwrite the firstName, lastName and fullName.

The reason we use a stepped process in _syncUserProfile() is so that we can handle and issues with applying attributes or fields. Using setAttributes() means it's all-or-nothing.

All too happy to add an event here to help.

Braedan commented 4 weeks ago

Hey thanks for the swift response! I think you're likely right, if syncProfile is true, or even populateProfile I'd assume the source of truth should be coming from the SSO provider.

I agree it would suck to lose the current benefits _syncUserProfile() offers. I guess the alternative would be implementing the same conditional logic Craft uses in setAttributes(), but understandable it that's not something that's warranted.

An event would definitely help my use-case, though my current workaround works well enough that I'm not desperate for it.