[FIX] Custom Oauth store refresh and id tokens with expiresIn

[ralfbecker]

Closes #13693

[CLAassistant]

All committers have signed the CLA.

[ralfbecker]

@geekgonecrazy do you mind reviewing this

[geekgonecrazy]

Nice! I’m not sure however... if the refresh is ever even used?

[ralfbecker]

The ci/circleci test failures have nothing to do with this pull-request:

WARNING: The following packages cannot be authenticated!
  google-chrome-stable
E: There were unauthenticated packages and -y was used without --allow-unauthenticated

[ralfbecker]

@geekgonecrazy You mean if Rocket.Chat/Meteor uses refreshTokens, or that it need to be set in a different way? I'm setting it now, like the Google provider sets it.

Ralf

[geekgonecrazy]

No I mean. Now we have it.. cool! but where or how is the refresh token actually used by Rocket.Chat?

[ralfbecker]

The accessToken is registered with it's expiration. If we got an refreshToken, we can use that to get a new accessToken, after it's expired, without "bugging" the user again.
My expectation is that this is done in Meteor's or Rocket.Chat's existing OAuth code. Why else would the other providers register the refreshToken. I have not verified that!

Ralf

[geekgonecrazy]

Just a heads up by adding the registerAccessToken as part of this you have overlap with: #14113 opened just a bit before yours.

[ralfbecker]

Hmm, #14113 seems to do the same as my intended next pull request:
63f6e52

[ralfbecker]

I rebased this pull request in my repo on top of @knrt10 one: https://github.com/EGroupware/Rocket.Chat/commit/7aba9cad805d376aaef2c7a7e6901bc742df00ad

I searched through Rocket.Chat codebase for usage of expiresAt or refreshToken: I'm pretty sure they are NOT used at all :(

I think when Rocket.Chat receives an accessToken, eg. through custom OAuth, it's ok to verify it's currently valid by using the identity endpoint with the accessToken as Authorization: Bearer.
Rocket.Chat then stores, with this pull request, expiresAt and, if we receive one, the refreshToken.

When the next Api request with the same token arrives, you can either:
a) check it again with the identity endpoint or
b) check the stored expiresAt time and only re-check if it's past that time
So for Api access there is (only) less latency to again, refresh token does not help at all.

Other use case is the Rocket.Chat client and I believe that's what #13693 is talking about: after the initial OAuth authentication by the user, Rocket.Chat should not assume the user is authenticated for ever, but only for the lifetime of the token. If the token is expired, Rocket.Chat either needs to bug the user again for an other OAuth authentication, or try to automatically get a new accessToken through the refreshToken (from the server-side), which might fail in case the user revoked the refreshToken in the meantime because he eg. lost the device.

I think that's were Rocket.Chat has a problem: there is no way to cut off access by a lost client-device, in case an external OAuth server is used.

Ralf

[geekgonecrazy]

Seems you are at the same conclusion I reached.

I still think this is great to have added. But yes we may need to follow up with a pull request to tie rocket.chat token life time to oauth life time. But I have a feeling that will not be an easy task to track down

[geekgonecrazy]

Sorry some of these suggestions are only needed because of the conflict merging. 😊

[weand]

Seems you are at the same conclusion I reached.

I still think this is great to have added. But yes we may need to follow up with a pull request to tie rocket.chat token life time to oauth life time. But I have a feeling that will not be an easy task to track down

is there any issue available thats tracks progress for "pull request to tie rocket.chat token life time to oauth life time"