openshift / openshift-restclient-java

Other
78 stars 112 forks source link

Client refresh token #455

Closed mhagnumdw closed 4 years ago

mhagnumdw commented 4 years ago

Hi!

I keep a client instance with application scope. There comes a time when the token expires.

Could you recommend a way that I can get a new token? Without interfering / breaking the current request.

Flow example:

web request -> MyController -> MyOpenShiftService.updateSomething -> getClient().update(resource)

where

private IClient getClient() {

I would like to check if the client (IClient) is valid before calling the update method, thus avoiding an authentication error.

ps: please add the label: "help wanted"

adietish commented 4 years ago

Hi @mhagnumdw

what you suggest in terms of usecase seems completely valid but I'm not aware of a way to verify a token other than to login (implicitly when executing an operation). I might be missing something though. In the Eclipse tooling, that is using this library, we simply catch the exception when authentication fails and re-authenticate. Is this not possible for you?

mhagnumdw commented 4 years ago

Hi @adietish , thanks for your attention.

we simply catch the exception when authentication fails and re-authenticate

I have a class with several methods that make calls to several IClient methods. Is there an exception handler so that I don't have to define a try-catch on each call?

How do you re-authenticate?

I'm not aware of a way to verify a token

Taking a look at the API, I found the IClient.getAuthorizationContext().getExpiresIn() method. I thought it could be used to check if the token is still valid, but that attribute is not filled.

My workaround for my case: when obtaining the instance of IClient that I keep in a singleton, check if X minutes have passed since the last login (I know the expiration time of my OpenShift token), if it has passed I do the re-authentication like this:

IAuthorizationContext authContext = client.getAuthorizationContext();
authContext.setToken(null); // to force a re-authentication
((AuthorizationContext) authContext).setUser(null); // to force a re-authentication - I needed to cast :(
if (!authContext.isAuthorized()) { // implicitly will try to authenticate
    throw new JarvisException("User " + authContext.getUserName() + " without authorization to access OpenShift");
}
this.lastLogin = DateUtils.now();

I don't re-create the client because it takes a lot longer.

adietish commented 4 years ago

Hi @mhagnumdw

Taking a look at the API, I found the IClient.getAuthorizationContext().getExpiresIn() method. I thought it could be used to check if the token is still valid, but that attribute is not filled.

This happens when the auth context is built via the primary constructor that includes token, username and password but is missing the expires field. IMHO this is required when you're manually building a client where you have the token but are obviously missing the "expiresIn". I'd see a possibility to retrieve and set this property when authenticating. AuthenticatorInterceptor#getToken is where check to see if "expiresIn" exists in the response. Can you check this?

((AuthorizationContext) authContext).setUser(null); // to force a re-authentication - I needed to cast :(

I'm not fully sure why #setUser wasn't included in the interface. My guess is that it wasn't to disallow 3rd parties to set this central piece of data and only allow savvy users to do it. If the purpose of such a setter is to invalidate the context, I'd rather allow this via some explicit method like #invalidate. It could then kill the token and the user at the same time. Makes sense?

mhagnumdw commented 4 years ago

Hi @adietish !

AuthenticatorInterceptor#getToken is where check to see if "expiresIn" exists in the response. Can you check this?

image

image

If the purpose of such a setter is to invalidate the context, I'd rather allow this via some explicit method like #invalidate. It could then kill the token and the user at the same time. Makes sense?

Invalidating the context to force a re-authentication seems like a good thing, since it is not necessary to recreate the client (which takes some time).

adietish commented 4 years ago

Hi @mhagnumdw

If I get your screenshots right, the response headers wont contain an "expiresIn" value. I'm not fully sure though bcs the screenshot cuts off the end of the location value. I created PR #461 that introduces the explicit invalidation and it would be great if you could test it and give me feedbacks on it so that we can match your requirements.

Greetings Andre

mhagnumdw commented 4 years ago

Hi @adietish !

I'm not fully sure though bcs the screenshot cuts off the end of the location value.

Follows full version:

Location: https://oauth-openshift.apps.dev.xxxxxxxxxxx/oauth/token/implicit#access_token=token-here-but-omitted&expires_in=7200&scope=user%3Afull&token_type=Bearer

I created PR #461 that introduces the explicit invalidation and it would be great if you could test it and give me feedbacks on it so that we can match your requirements.

I will do that and return soon. Thank you!

mhagnumdw commented 4 years ago

I created PR #461 that introduces the explicit invalidation and it would be great if you could test it and give me feedbacks on it so that we can match your requirements.

@adietish , it worked really well. Tks!

adietish commented 4 years ago

@mhagnumdw

Location: https://oauth-openshift.apps.dev.xxxxxxxxxxx/oauth/token/implicit#access_token=token-here-but-omitted&expires_in=7200&scope=user%3Afull&token_type=Bearer

Looks like we have an "expires_in" value here that we can exploit. Let me do that in a separate issue/PR: #463