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

KNOX-3038 - The expires_in field in OAuthResource response returns the configured token TTL. #907

Merged
merged 1 commit into from
May 10, 2024

Conversation

smolnar82
Copy link
Contributor

@smolnar82 smolnar82 commented May 9, 2024

What changes were proposed in this pull request?

To honor the contract of the existing expiration time in the database for Knox Tokens, I removed the override in the new OAuthResource class. Thus, everything that depends on this field will be the same as in the case of our "regular" tokens (token eviction is the most important piece here).
To indicate the actual OAuth token lifetime, I changed the meaning of the expires_in field in the JSON response: that's the configured TTL in seconds.

How was this patch tested?

Updated JUnit tests and executed manual testing:

$ curl -ik -X POST -H "Content-Type: application/x-www-form-urlencoded" --data "grant_type=client_credentials" --data "client_id=$CLIENT_ID" --data-urlencode "client_secret=$CLIENT_SECRET" https://localhost:8443/gateway/tokenbased/oauth/v1/token
HTTP/1.1 200 OK
Date: Thu, 10 May 2024 23:46:18 GMT
Content-Type: application/json
Content-Length: 1098

{"access_token":"eyJqa...0ijh_g","refresh_token":"a36bafd4...9491-7e17e710a004","issued_token_type":"urn:ietf:params:oauth:token-type:access_token","token_type":"Bearer","expires_in":10368000}

The tokenabased topology was configured with knox.token.ttl = 10368000000. As you can see, the expires_in field in the response was populated as expected (converted the given TTL milliseconds to seconds).

Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -90,6 +91,7 @@ public Response getAuthenticationToken() {
map.put(ISSUED_TOKEN_TYPE, ISSUED_TOKEN_TYPE_ACCESS_TOKEN_VALUE);
// let's use the passcode as the refresh token
map.put(REFRESH_TOKEN, passcode);
map.put(LIFETIME, getTokenLifetimeInSeconds());
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am reading this correctly, then returning the actual value in the response as lifetime_secs would be unexpected by OAuth flow consumers/clients. They will be looking for expires_in.

I think what we need to do is calculate both the existing expires_in which millis since epoch as well as this OAuth expires_in which is lifetime_secs and return one in the response and store the other in the token metadata for reaping and monitoring, etc.

…e configured token TTL.

Additionally, removed the override of the getExpiry() method because in the database the meaning of expiration time must remain the same as in the parent class.
@smolnar82 smolnar82 changed the title KNOX-3038 - Added a new element in OAuthResource response called lifetime_secs KNOX-3038 - The expires_in field in OAuthResource response returns the configured token TTL. May 9, 2024
Copy link
Contributor

@lmccay lmccay left a comment

Choose a reason for hiding this comment

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

LGTM!

+1

@smolnar82 smolnar82 merged commit f7fcc7a into apache:master May 10, 2024
2 checks passed
@smolnar82 smolnar82 deleted the KNOX-3038 branch May 10, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants