8140 fixed username validation federated account creation

[mderuijter]

What this PR does / why we need it:
Fixes username validation for federated account creation
Which issue(s) this PR closes:

Closes #8140

Special notes for your reviewer:
None
Suggestions on how to test this:
Please see steps explained in the related issue
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
No
Additional documentation:

[mderuijter]

@djbrooke It seems CI is failing, I made sure to merge the latest from develop. Any idea what is causing it?

[qqmyers]

@mderuijter,
It’s a failure on the new Curation label API (below). It looked to me like you had most of the fixes that went in for that feature but I’d suggest merging with develop again to see if that makes it go away.

-- Jim

Error
Expected status code <200> doesn't match actual status code <400>.
Stacktrace
java.lang.AssertionError:
Expected status code <200> doesn't match actual status code <400>.
at edu.harvard.iq.dataverse.api.DatasetsIT.testCurationLabelAPIs(DatasetsIT.java:2404)
Standard Output
{"status":"OK","data":{"user":{"id":125,"userName":"user4ef6a686"},"authenticatedUser":{"id":125,"identifier":"@user4ef6a686","displayName":"user4ef6a686 user4ef6a686","firstName":"user4ef6a686","lastName":"user4ef6a686","email":"user4ef6a686@mailinator.com","superuser":false,"deactivated":false,"persistentUserId":"user4ef6a686","createdTime":"2021-10-12T17:30:13Z","lastLoginTime":"2021-10-12T17:30:13Z","authenticationProviderId":"builtin"},"apiToken":"502666a6-6973-4e1d-8033-7f0a063f6e53"}}
{"status":"OK","data":{"id":166,"alias":"dv66eb818c","name":"dv66eb818c","dataverseContacts":[{"displayOrder":0,"contactEmail":"9880534a@mailinator.com"}],"permissionRoot":true,"dataverseType":"UNCATEGORIZED","ownerId":1,"creationDate":"2021-10-12T17:30:13Z"}}
Standard Error
Oct 12, 2021 5:30:13 PM edu.harvard.iq.dataverse.api.UtilIT createRandomUser
INFO: Creating random test user user4ef6a686
Oct 12, 2021 5:30:13 PM edu.harvard.iq.dataverse.api.UtilIT getUsernameFromResponse
INFO: Username found in create user response: user4ef6a686
Oct 12, 2021 5:30:13 PM edu.harvard.iq.dataverse.api.UtilIT getApiTokenFromResponse
INFO: API token found in create user response: 502666a6-6973-4e1d-8033-7f0a063f6e53
Oct 12, 2021 5:30:13 PM edu.harvard.iq.dataverse.api.UtilIT getAliasFromResponse
INFO: Alias found in create dataverse response: dv66eb818c

[sekmiller]

Would it make sense to put the userNameFound method into the UserNameValidator? I know we probably can't put it into the isValid method because we want to show different error messages, but it might be good for code consolidation.

[djbrooke]

@mderuijter - what do you think about the feedback from @sekmiller above?

[mderuijter]

@sekmiller @djbrooke it does make sense to combine the two, let me put it on the to do list

[qqmyers]

@mderuijter - #8151 fixes the test failure you saw previously. I'm not sure how/why your branch doesn't show a failure now as the test is truly broken. In any case - merging with dev now should be a real fix.

[scolapasta]

What the ConstraintValidatorContext removed because it was never used? If so, @sekmiller do you happen to recall why it may have been there? And if so should we also then remove it from the isValid method above? (this would mean finding all calls to it, of course)

[mderuijter]

@scolapasta It was removed because it was not being used indeed.

[sekmiller]

It seems not to have been used at all and should probably be removed from isValid. Not sure why it was in there in the first place. It looks like Sarah wrote the original code.

[scolapasta]

@mderuijter since there's no reason to have the context in the isValid method, would you be able to remove it there (and refactor calls to it) as well?

[mderuijter]

@scolapasta I see this will result in removing the entire implements ConstraintValidator<ValidateUserName, String> Interface structure as well, which leads to a related problem in ValidateUserName.java as well. Which in turn is used in BuiltinUser.java line 48. So eventhough the context parameter is not used, the surrounding structure is. I'm not entirely sure how to refactor that properly...

[scolapasta]

ok! that makes sense. I went ahead and added a comment in order to clarify this for when we look at this down the line.

[mderuijter]

Would it make sense to put the userNameFound method into the UserNameValidator? I know we probably can't put it into the isValid method because we want to show different error messages, but it might be good for code consolidation.

@sekmiller I had a look at the code again and I'm assuming you mean the method call of the AuthenticationService at

dataverse/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2FirstLoginPage.java

Line 242 in 6f47e5d

boolean userNameFound = authenticationSvc.identifierExists(userName);

Moving that to the UserNameValidator would either mean turning UserNameValidator into a service bean or passing the AuthenticationService as a parameter and creating a separate method that calls authenticationSvc.identifierExists(userName) so you can return different FacesMessages.

I'm not sure there's a lot to win here by moving that around to be honest

[sekmiller]

OK @mderuijter, point taken. My hope was to have all of the validation logic in one place, but I see the pitfalls. I'll approve the PR but let Gustavo make the final call on passing to QA.

[coveralls]

Coverage decreased (-0.002%) to 19.016% when pulling 5b7d028 on mderuijter:8140-fixed-username-validation-federated-account-creation into c0d06a5 on IQSS:develop.