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

Improves token manager's refresh token handling #541

Merged
merged 9 commits into from
Jan 14, 2025

Conversation

mdanish98
Copy link
Contributor

@mdanish98 mdanish98 commented Nov 25, 2024

Description of Changes

Fixes the issue with retrieving the access token from the refresh token after SSO has reached max.

Related Issue

Closes #530

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
@mdanish98 mdanish98 marked this pull request as ready for review December 23, 2024 08:39
@aaronzi aaronzi self-requested a review January 8, 2025 10:42
Copy link
Member

@aaronzi aaronzi left a comment

Choose a reason for hiding this comment

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

Thank you for the improvements. I added some small remarks.

*/
public synchronized String getAccessToken() throws IOException {
long currentTimeMillis = System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

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

Using System.currentTimeMillis() can be susceptible to system clock changes, which might lead to unexpected behavior.

I would suggest using System.nanoTime() for measuring elapsed time or leverage a time abstraction (e.g., injecting a Clock instance) to make the code more testable and resilient to system clock changes.

* @throws IOException
* if an error occurs while retrieving the token
*/
public synchronized String getAccessToken() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

You are declaring the entire function synchronized for thread safeness. For performance reasons this could be optimized. Here is an example:

public String getAccessToken() throws IOException {
    long currentTimeMillis = System.currentTimeMillis();

    if (accessToken != null && currentTimeMillis < accessTokenExpiryTime) {
        return accessToken;
    }

    synchronized (this) {
        // Re-check within synchronized block
        if (accessToken != null && currentTimeMillis < accessTokenExpiryTime) {
            return accessToken;
        }

        // Proceed with token refresh or retrieval
        // ...
    }
}

Comment on lines 90 to 103
if (refreshToken != null && currentTimeMillis < refreshTokenExpiryTime) {
try {
return updateTokens(accessTokenProvider.getAccessTokenResponse(tokenEndpoint, refreshToken), currentTimeMillis);
} catch (IOException e) {
throw new AccessTokenRetrievalException("Error occurred while retrieving access token" + e.getMessage());
throw new AccessTokenRetrievalException("Error occurred while retrieving access token: " + e.getMessage());
}
}
}

try {
return requestAccessToken(accessTokenProvider.getAccessTokenResponse(tokenEndpoint));
try {
return updateTokens(accessTokenProvider.getAccessTokenResponse(tokenEndpoint), currentTimeMillis);
} catch (IOException e) {
throw new AccessTokenRetrievalException("Error occurred while retrieving access token" + e.getMessage());
throw new AccessTokenRetrievalException("Error occurred while retrieving access token: " + e.getMessage());
}
}

private String requestAccessToken(AccessTokenResponse accessTokenResponse) throws IOException {
AccessToken accessTokenObj = accessTokenResponse.getTokens().getAccessToken();
accessToken = accessTokenObj.getValue();
accessTokenExpiryTime = accessTokenObj.getLifetime();

RefreshToken refreshTokenObj = accessTokenResponse.getTokens().getRefreshToken();

if (refreshTokenObj != null) {
refreshToken = refreshTokenObj.getValue();
refreshTokenExpiryTime = System.currentTimeMillis() + (30L * 24L * 60L * 60L * 1000L);
}

return accessToken;
}

}
Copy link
Member

Choose a reason for hiding this comment

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

You have a duplication of logic here for both the refresh and direct retrieval attempts. This could be a possible way of refactoring this. What do you think?

private String refreshAccessToken(long currentTimeMillis) throws IOException {
    return updateTokens(accessTokenProvider.getAccessTokenResponse(tokenEndpoint, refreshToken), currentTimeMillis);
}

private String obtainNewAccessToken(long currentTimeMillis) throws IOException {
    return updateTokens(accessTokenProvider.getAccessTokenResponse(tokenEndpoint), currentTimeMillis);
}

public synchronized String getAccessToken() throws IOException {
    long currentTimeMillis = System.currentTimeMillis();

    if (accessToken != null && currentTimeMillis < accessTokenExpiryTime) {
        return accessToken;
    }

    if (refreshToken != null && currentTimeMillis < refreshTokenExpiryTime) {
        try {
            return refreshAccessToken(currentTimeMillis);
        } catch (IOException e) {
            throw new AccessTokenRetrievalException("Error occurred while refreshing access token", e);
        }
    }

    try {
        return obtainNewAccessToken(currentTimeMillis);
    } catch (IOException e) {
        throw new AccessTokenRetrievalException("Error occurred while obtaining new access token", e);
    }
}

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Copy link
Member

@aaronzi aaronzi left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronzi aaronzi merged commit 028a161 into eclipse-basyx:main Jan 14, 2025
27 checks passed
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.

[BUG] TokenManager does not re-init expired sessions
2 participants