Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: generate email if it is the empty string on external login #2868

Merged
merged 1 commit into from
May 29, 2024

Conversation

mikeroda
Copy link
Contributor

@mikeroda mikeroda commented May 3, 2024

If the email is the empty string, treat it as null and generate one.

Change-Id: I379382bb07c5e01766ff1ce131f560eabfef253b
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187543759

The labels on this github issue will be updated when the story is started.

@mikeroda
Copy link
Contributor Author

mikeroda commented May 3, 2024

Without the change, the test case will throw an exception:

java.lang.IllegalArgumentException: Email is required
at org.springframework.util.Assert.hasText(Assert.java:289)
at org.cloudfoundry.identity.uaa.user.UaaUser.(UaaUser.java:152)

Because UaaUser has Assert.hasText(prototype.getEmail(), "Email is required") which is inconsistent with the null check in ExternalLoginAuthenticationManager.

@Tallicia Tallicia added in progress in_review The PR is currently in review labels May 8, 2024
@Tallicia Tallicia self-assigned this May 8, 2024
@@ -380,8 +380,8 @@ protected UaaUser getUser(Authentication request, AuthenticationData authenticat
Object verifiedObj = claims.get(emailVerifiedClaim == null ? "email_verified" : emailVerifiedClaim);
boolean verified = verifiedObj instanceof Boolean ? (Boolean)verifiedObj: false;

if (email == null) {
email = generateEmailIfNull(username);
if (!StringUtils.hasText(email)) {
Copy link
Member

@peterhaochen47 peterhaochen47 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use !StringUtils.hasText(email) here but StringUtils.isEmpty(email) in ExternalLoginAuthenticationManager.java? Why the different logics?

It looks like !StringUtils.hasText is a narrower/stricter validation? It also evaluates to true if the string contains only whitespace(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only because this one uses the Spring StringUtils and isEmpty is deprecated. ExternalLoginAuthenticationManager uses the Apache StringUtils. I didn't want to affect other lines of code by changing one or the other, and I didn't want to use a deprecated method.

@peterhaochen47
Copy link
Member

Also, what is the issue in real life? When logging in with an external IDP, the email value we get from the IDP would be an empty string & the user will fail to log in due to "Email is required" error? Why would the IDP has an empty string as the email (instead of just not having it)? Is the real issue here the operator didn't set up the attributeMappings config correctly for UAA API?

@mikeroda
Copy link
Contributor Author

Also, what is the issue in real life? When logging in with an external IDP, the email value we get from the IDP would be an empty string & the user will fail to log in due to "Email is required" error? Why would the IDP has an empty string as the email (instead of just not having it)? Is the real issue here the operator didn't set up the attributeMappings config correctly for UAA API?

The issue is that it's treated inconsistently. A null email will result in one being generated. An empty string will throw an exception that email is required. In reality an attributeMapping for email is not required, that why we can generate one. So we should be consistent about how we do that.

@peterhaochen47 peterhaochen47 requested a review from a team May 24, 2024 22:53
@Tallicia Tallicia added the accepted Accepted the issue label May 29, 2024
@peterhaochen47
Copy link
Member

Hi @mikeroda, approved, please merge.

@mikeroda
Copy link
Contributor Author

Hi @mikeroda, approved, please merge.

I don't have merge rights.

@peterhaochen47 peterhaochen47 merged commit eedb443 into cloudfoundry:develop May 29, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in_review The PR is currently in review
Projects
Development

Successfully merging this pull request may close these issues.

4 participants