Skip to content

feat: add authenticator invalidate (SHOP-231)#134

Open
vsolovei-smartling wants to merge 1 commit intomasterfrom
SHOP-231-invalidate
Open

feat: add authenticator invalidate (SHOP-231)#134
vsolovei-smartling wants to merge 1 commit intomasterfrom
SHOP-231-invalidate

Conversation

@vsolovei-smartling
Copy link
Copy Markdown
Contributor

Having invalidation will allow to retry invalid token

Comment on lines +119 to +124
public synchronized void invalidate()
{
this.authentication = null;
this.expiresAt = -1;
this.refreshExpiresAt = -1;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's not called by anyone, isn't?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

that filter is only for logging. your http client from sdk already gets 401 response isn't? why can't you handle this 401 in sdk directly?

Comment on lines -123 to +130
this.refreshExpiresAt = authentication.getRefreshExpiresIn() * 100 + System.currentTimeMillis();
this.refreshExpiresAt = authentication.getRefreshExpiresIn() * 1000 + System.currentTimeMillis();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like the real fix is this one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is THE fix, it will reduce full re-authentication count, the only downside of having 100 vs 1000 is the frequency of full re-authentications vs refreshes.

@vsolovei-smartling
Copy link
Copy Markdown
Contributor Author

Will go with handling in the sdk

@vsolovei-smartling
Copy link
Copy Markdown
Contributor Author

After some consideration, I'd still like to go ahead with the PR, even if the invalidate will end up unused. Having full re-auth vs refreshing might not be a performance issue, but the *100 is really confusing

@@ -116,19 +116,26 @@ boolean isRefreshable()
return refreshExpiresAt > clock.currentTimeMillis();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add a gap period to current ms here also.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since we are fixing the timeunit conversion, we need to have a time gap here the same as for the expiresAt. So it should be smth like: refreshExpiresAt > clock.currentTimeMillis() + REFRESH_BEFORE_EXPIRES_MS;

@asmirnova-smartling
Copy link
Copy Markdown

Concrete Impact
Say Keycloak returns refreshExpiresIn = 1800 (30 minutes, a typical value):

Expected: refresh window = 1800 × 1000 = 1,800,000 ms (30 min)
Actual: refresh window = 1800 × 100 = 180,000 ms (3 min)
So after ~3 minutes, isRefreshable() returns false even though the refresh token is actually valid for 27 more minutes.

What Happens When isRefreshable() Returns False Prematurely
Looking at refreshOrRequestNewAccessToken():

Access token expires (or is within 90s of expiry) → isValid() returns false
isRefreshable() is checked — returns false too early due to the bug
Instead of refreshing the token (lightweight call), it falls through to full re-authentication (getAccessTokenInternal()) with userIdentifier + userSecret

Symptoms
Area Effect
Performance Unnecessary full authentication calls to Keycloak instead of cheaper refresh calls. Every SDK client re-authenticates ~10x more often than needed.
Keycloak load Higher load on Keycloak's token endpoint — password grant is heavier than refresh grant (involves credential validation).
Reliability If Keycloak is under pressure, full auth is more likely to fail/timeout than a refresh. More frequent auth = more exposure to transient failures.
Functional correctness No user-visible errors — authentication still works, it's just wasteful. The fallback to full re-auth masks the bug silently.
Logging You'd see "Requesting new token." in logs far more often than "Going to refresh access token." — this is the telltale sign.

{
this.authentication = api.authenticate(new AuthenticationRequest(userIdentifier, userSecret));
this.expiresAt = authentication.getExpiresIn() * 1000 + System.currentTimeMillis();
this.refreshExpiresAt = authentication.getRefreshExpiresIn() * 100 + System.currentTimeMillis();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also I would consider replacing usage of System.currentTimeMillis with usage of the Clock.
And also replace custom Clock interface with standard java.util.Clock instead (since it's package private I don't think this will be a breaking change).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What are benefits of usage Clock over plain System.currentTimeMillis?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants