Spring Security clockSkew Javadoc is not consistent with implementation

Spring Security 5.5.1

In each of the inner classes in the OAuth2AuthorizedClientProviderBuilder (such as the PasswordGrantBuilder) the comment on the use of the clockskew is PasswordGrantBuilder.clockSkew() "An access token is considered expired if it's before {@code Instant.now(this.clock) - clockSkew}."

However, the use of the clockskew in the PasswordOAuth2AuthorizedClientProvider and other OAuth2AuthorizedClientProvider implementations does not use the clock skew in this way and instead calculates if the token is expired using

PasswordOAuth2AuthorizedClientProvider.hasTokenExpired()

private boolean hasTokenExpired(OAuth2Token token) {
    return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
}

The calculation does not seem to be the correct use and should match the documentation and be

private boolean hasTokenExpired(OAuth2Token token) {
    return token.getExpiresAt().isBefore(this.clock.instant().minus(this.clockSkew));
}

Comment From: eleftherias

Thanks for reaching out @andywhite27. This was changed as part of gh-7511, take a look at that issue for more details.

Comment From: andywhite27

Hi @eleftherias ,

I understand the reply although I don't agree! However, that aside there is still an issue here with the documentation which is the first paragraph of my original note. The documentation on each of the classes in OAuth2AuthorizedClientProviderBuilder are incorrect. If the code is not going to be fixed then at least the documentation should be please.

"In each of the inner classes in the OAuth2AuthorizedClientProviderBuilder (such as the PasswordGrantBuilder) the comment on the use of the clockskew is PasswordGrantBuilder.clockSkew() "An access token is considered expired if it's before {@code Instant.now(this.clock) - clockSkew}."

Comment From: eleftherias

Thanks @andywhite27, I have reopened the issue. Are you interested in submitting a PR to update the documentation?

Comment From: qavid

@eleftherias I have prepared PR #10358. Please take a look.

Also please note, that in implementations of hasTokenExpired (e.g. PasswordOAuth2AuthorizedClientProvider#hasTokenExpired) is possible NPE in situations when OAuth2Token#getExpiresAt() returns null. Do you think we should open new issue?