Skip to content

Check user code expiry and invalidity #1997

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

Conversation

antoinelauzon-bell
Copy link
Contributor

It ensures that a user code is neither expired nor invalidated during the verification step. See gh-1894 and gh-1977 for more details.

Notes:

  • invalidat_grant appears to be the expected error code (see  RFC 6749, Section 5.2). It might be useful to distinguish these exceptions though (e.g. by using subclasses of OAuth2AuthenticationException).
  • A small gap remains where a user could verify a user code in the final seconds before it expires, leasing to an expired device code on the next poll from the initial device. This scenario would require very unlucky timing.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 5, 2025
@OrangeDog
Copy link

Would it not be better to just check isActive()? Not only is it less code, but it also covers cases you have missed.

if (this.logger.isTraceEnabled()) {
this.logger.trace("User code is expired");
}
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);

Choose a reason for hiding this comment

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

Is it worth providing more context on the error..

Suggested change
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT, "User code is expired.", null);
throw new OAuth2AuthenticationException(error);

if (this.logger.isTraceEnabled()) {
this.logger.trace("User code is invalided");
}
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);

Choose a reason for hiding this comment

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

Is it worth providing more context on the error..

Suggested change
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT, "User code is invalidated.", null);
throw new OAuth2AuthenticationException(error);

@jgrandja
Copy link
Collaborator

@antoinelauzon-bell Apologies for the late response but I've been busy preparing for the 1.5 release this Tuesday. I will likely review this next week.

@jgrandja jgrandja self-assigned this May 15, 2025
@jgrandja jgrandja added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 15, 2025
@jgrandja jgrandja added this to the 1.3.7 milestone May 15, 2025
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @antoinelauzon-bell. Please see review comment.

Also, please update copyright year for all files and rebase off of 1.3.x.

Thanks.

@@ -109,6 +109,21 @@ public Authentication authenticate(Authentication authentication) throws Authent
this.logger.trace("Retrieved authorization with user code");
}

OAuth2Authorization.Token<OAuth2UserCode> userCode = authorization.getToken(OAuth2UserCode.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please combine the 2 checks as follows:

if (!userCode.isActive()) {
	if (!userCode.isInvalidated()) {
		// Invalidate the user code
		authorization = OAuth2Authorization.from(authorization).invalidate(userCode.getToken()).build();
		this.authorizationService.save(authorization);
	}
	throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
}

And adjust the tests as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, see last commit with the changes. A log might be relevant (see @colin-riddell's comment above). Let me know what you think.

@antoinelauzon-bell antoinelauzon-bell changed the base branch from main to 1.3.x June 2, 2025 12:40
jgrandja added a commit that referenced this pull request Jun 3, 2025
@jgrandja
Copy link
Collaborator

jgrandja commented Jun 3, 2025

Thanks for the updates @antoinelauzon-bell. This is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants