Skip to content

StandardClaimAccessor.getEmailVerified returns null if the claim email_verified does not exist #12587

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

Closed
gotson opened this issue Jan 26, 2023 · 4 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply

Comments

@gotson
Copy link

gotson commented Jan 26, 2023

Expected Behavior

The documentation of the method states:

Returns true if the user's e-mail address has been verified (email_verified), otherwise false.

Current Behavior

If the claim email_verified doesn't exist, returns null.

Context

A user of my application reported a NPE when trying to use OIDC with Azure AD.

IMHO, either the documentation should be enhanced to clarify that getEmailVerified can return null if the claim is not present, or the code should handle this in a try/catch block to return false if the claim does not exist.

@gotson gotson added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jan 26, 2023
@marcusdacoregio marcusdacoregio added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jan 26, 2023
@marcusdacoregio marcusdacoregio added this to the 5.7.7 milestone Jan 26, 2023
@marcusdacoregio marcusdacoregio added the status: waiting-for-triage An issue we've not yet triaged label Jan 26, 2023
@marcusdacoregio
Copy link
Contributor

Hi @gotson, thanks for the report.

Are you interested in submitting a PR that fixes it? The PR would contain a test to reproduce the error and that is fixed after your solution.

@gotson
Copy link
Author

gotson commented Jan 26, 2023

Hi @gotson, thanks for the report.

Are you interested in submitting a PR that fixes it? The PR would contain a test to reproduce the error and that is fixed after your solution.

Hi, which solution do you think should be implemented, fixing the doc, or fixing the behavior? I can send a PR.

@gotson
Copy link
Author

gotson commented Jan 27, 2023

I thought about this a bit more, and folding the absence of claim as false is probably not a good choice, as we may want to differentiate between whether the claim is present (and true or false), or whether it's missing. In which case the Boolean would be null.

Does Spring Security uses Optional or @Nullable to provide hints on nullability?

@marcusdacoregio
Copy link
Contributor

Returning false if the claim does not exist might be misleading indeed, and, returning null also align with the behavior of the various other methods in the class. Spring Security does not use optional because #9641, and @Nullable is being discussed here.

I think @Nullable could be a good idea, therefore I'll close this to keep the discussion in #8389.

@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jan 27, 2023
@marcusdacoregio marcusdacoregio self-assigned this Jan 27, 2023
@jgrandja jgrandja removed this from the 5.7.7 milestone Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants