Don't attempt username sync when user was already matched by username

needs a reroll

and against RC1.

Thank you. The most recent patch for 8.x-3.x in #4 worked well. For those searching for an answer based on the error message from this issue: "Error synchronizing username: an account with this username already exists."

Re-rolling patch against RC3. Works as mentioned.

I can confirm, that #7 is working with 8.x-3.0-rc3 and 8.x-3.0 stable.

I was about to RTBC this but concerned about the complexity it introduced. Throwing up an alternative patch that checks the user account coming in vs the existing account instead of the currentUser on class instantiation.

I think there is something wrong with the branch testing because the failure is unrelated.

A note that we could fix but want to gather feedback. With the patch I provided, the username will override on the initial sync. That's a feature in my case but I think that is covered in the previous patch. Thoughts? Should we deal with that too?

Actually I should do feature parity (testbot will still fail)

Here's a fix for the failing unit test in HEAD #2969420: Tests failing in HEAD due to missing mocked method

@bradjones1 thanks for chiming in. I think #12 should deal with both cases. I would have just used the other patch but while describing it to someone on my team I realized it could be simplified so thought to propose the alternative that doesn't have to track the variable between functions(the hope).

I've been using the other patch in production for six months (RTBC!), but #12 does the trick for me as well.

Looks good to me as well. I have not required it since I discovered the checkbox "Enable this user to leverage SAML authentication" under the user's account.

...or add simplesamlphp_auth as a provider in the authmap table. As it explicitly says under User Provisioning. 🤦

Same as https://www.drupal.org/project/simplesamlphp_auth/issues/2984056 isn't it ?

I added a small extra change to patch #12 to correct the log message.

This is a bit confusing. it does make more sense to compare against $account, but at this point, it *should* be the same as $this->currentUser(), as that the event happens after logging.

I don't understand the move of $existing though, and what that logic already does. Without that, the patch is now identical to #2984056: SimplesamlphpDrupalAuth's synchronizeUserAttributes() attempts to match against current user vs. found $account

This patch no longer works with version 8.x-3.1 so I'm submitting this new version of the patch.

@Berdir I forget why I needed this, but it may be because I'm using masquerade or something. Also to deal with variables you are passing to the function instead of looking outside for potentially same value.

But the $existing = TRUE; move up is critical because if ($existing_account = reset($account_search)) { already determines that it exists with that name and the if ($account->id() != $existing_account->id()) { is only negating that the uid's don't match which doesn't mean $existing = TRUE;

Here's a reroll because the other part was committed upstream, see #24 for explanation of why this is needed.

Hm, still not sure I fully understand this, still somewhat new to this project.

A test would be helpful to cover this use case, there are also several other open issues about matching where that would help too. There is an existing unit test in \Drupal\Tests\simplesamlphp_auth\Unit\Service\SimplesamlphpDrupalAuthTest::testSynchronizeUserAttributes(), but that only covers the case when there is no existing user with that name.

We could either add additional unit tests with two more variations: once that returns the same user entity, with the same ID and one that returns a different one with a different ID, and then assert what exactly should be set in which case. Alternatively, define a kernel test that requires less mocking. Unit tests tend to be harder to set up, but most of the work is already done, we just need to create variations of it.

Hm, still not sure I fully understand this, still somewhat new to this project.

I know the feeling!

I think the test may help, thanks for pointing out which one would be good to tackle.

Would it be ok to put a masquerade integration test?

Not quite sure how a masquerade integration test is relevant for this specific issue, but in general, sure, I'm also using patches for masquerade and this project to make them play together, being able to test that would be interesting.

Patch in #25 dropped the logging parameter changes, attached patch restores it. Since tests on both #23 and #25 pass, I think this needs a unit test before it is committed.

#29 is working with the 4.0.0-rc1 release.

A test would have been nice, but didn't happen in several years :-(