transistorsoft / cordova-background-geolocation-lt

The most sophisticated background location-tracking & geofencing module with battery-conscious motion-detection intelligence for iOS and Android.
http://www.transistorsoft.com/shop/products/cordova-background-geolocation
Other
655 stars 277 forks source link

Refresh token rotation (with single-use refresh tokens) doesn't seem to be supported #1256

Closed doubliez closed 2 years ago

doubliez commented 3 years ago

Your Environment

Only putting the relevant part here which is the authorization config:

authorization: {
  strategy: 'JWT',
  accessToken: initialAccessToken,
  refreshToken: initialRefreshToken,
  refreshUrl: `{authServer}/oauth2/token`,
  refreshPayload: {
    grant_type: 'refresh_token',
    refresh_token: '{refreshToken}'
  }
}

Expected Behavior

After refreshing the token, the refreshToken should be extracted from the response and used in the next refresh request.

Actual Behavior

The initialRefreshToken is used for all requests to refreshUrl, which doesn't work in my case since the refresh tokens issued by the OAuth2 server are single-use (to prevent replay attacks).

So what happens is that the plugin is able to successfully perform a token refresh only once, and it breaks after the second token refresh and remains broken until opening the app and supplying a new refresh token from the JavaScript code.

The token refresh response from the OAuth2 server looks like:

{
    "access_token": "...",
    "expires_in": 3600,
    "id_token": "...",
    "refresh_token": "...",
    "scope": "openid offline_access",
    "token_type": "bearer"
}

The plugin should extract the value of refresh_token and use that for the next token refresh call.

Steps to Reproduce

Context

Debug logs

Logs ``` PASTE_YOUR_LOGS_HERE ```
christocracy commented 3 years ago

Does your refreshToken take the same form XXX.YYY.ZZZ as your accessToken?

Are you observing plugin logs?

doubliez commented 3 years ago

@christocracy Here is an example refresh token:

_B1V86b9AQQdZVMLURkZtESNEqKsEyX0yJeYjKK5ARQ.oF-70Wg2M1W97ewBLJD_Qm-izYtC13m7k720VF5XdYC

I didn't look at plugin logs, I can do that tomorrow. I was just looking at the requests coming to the auth server and I was able to confirm that the same refresh token was sent by the plugin on the second refresh call, which is incorrect. The auth server returns 400 code when a refresh token is re-used.

christocracy commented 3 years ago

The SDK uses the following Regular Expression to determine a refreshToken:

/^[A-Za-z0-9-_=]+$/

Your refreshToken takes the form XXX.YYY which doesn't match the Regular Expression above.

Can you modify your code to not use . character in the refreshToken?

doubliez commented 3 years ago

@christocracy Ok that explains it then. Unfortunately I cannot change the refresh token format. We use ORY Hydra (https://www.ory.sh/hydra/) as our OAuth2 server and it's not possible to change the format of the refresh tokens it issues.

So the SDK is not looking at the keys at all? It's only matching on the values?

It would be great if we could tell the SDK where the tokens are located in the payload, rather than letting it magically find them.

Something like that maybe, using JSONPath syntax to tell the SDK where to look:

authorization: {
  ...authConfig,
  accessTokenInResponse: '$.access_token',
  refreshTokenInResponse: '$.refresh_token'
}
christocracy commented 3 years ago

So the SDK is not looking at the keys at all? It's only matching on the values?

Correct. The SDK "tastes" the response with regular-expressions.

It would be great if we could tell the SDK where the tokens are located in the payload

It makes the API too complex.

doubliez commented 3 years ago

So if you don't want to add extra config options, would it be possible to find the tokens based on key name?

In addition to the regex matching on values, also try to find the following keys in the object:

christocracy commented 3 years ago

I will experiment with modifying the regular-express to match XXX.YYY for refreshToken

/^[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_=]+?$/
doubliez commented 3 years ago

Great thanks, that would work too. There is always only one dot in the refresh token issued by Hydra.

doubliez commented 3 years ago

Actually there would be a problem with that I think, because the access token is also in the form XXX.YYY (it's not a JWT token, we use opaque tokens).

I'm curious what regex do you use for the access token, because this part worked fine even though the token is not a JWT.

christocracy commented 3 years ago

JWT Regular expression:

/^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*$/

I think you're going to have to not use Config.authorization. You will have to manually manage your authorization tokens using Config.headers and listen to onHttp for 401 responses as your cue to refresh your token.

doubliez commented 3 years ago

Ok so the second dot is optional in the regex, that's why it matches our access token. It would also match the refresh token but the access token comes first in the payload that's probably why it grabs it correctly.

I'm not sure if handling 401 in onHttp would work in the case the app has been terminated? My understanding is the JavaScript listeners wouldn't be triggered?

We need to be able to reliably send locations to our backend even when the app is not running, so ideally the SDK should refresh the token on its own. And we need to support both iOS and Android.

christocracy commented 3 years ago

I'm not sure if handling 401 in onHttp would work in the case the app has been terminated?

For Android, that is correct. For Android, you will have to use Android Headless Mode. You will have to use the plugin's native API to update your Config.headers when you detect an HTTP 401.

Start by setting up Android Headless Mode and I will show you native code to update your headers.

doubliez commented 3 years ago

Ok I'll look into that. But I think the Config.authorization is really close to be working and it would be easier if this would just work... It doesn't sound like a lot of work for you to just look for specific keys in the payload in addition to the existing regexes that you have? I think most OAuth2 servers would use access_token/refresh_token as keys (or in camelCase) and it should be safe to look for that? And in my opinion more reliable than doing regex on values.

Would the onHttp listener trigger when the app is in the background (but not terminated)? On iOS and Android?

christocracy commented 3 years ago

I think most OAuth2 servers would use access_token/refresh_token

Looking at the code, it seems I have a couple of unused regular expression where I'd planned to have the plugin to "taste" the keys as well:

private static Pattern ACCESS_PATTERN              = Pattern.compile("^access|auth");
private static Pattern REFRESH_OR_RENEW_PATTERN    = Pattern.compile("^renew|refresh");

Have you not purchased a license? I can push this to the private repo for you test but only paying customers get access to latest updates via the private repo.

doubliez commented 3 years ago

@christocracy Sorry for the delay, I was waiting for our IT team to buy the license. Now we have it, and it's linked to my GitHub user.

I'd be happy to test that, these regexes for the keys should work fine for us.

christocracy commented 3 years ago

Great. I will push a branch for you to the private repo tomorrow.

In the meantime, see the setup instructions in private repo and get your app reconfigured to install from private repo.

christocracy commented 3 years ago

Ok, I've pushed a branch #authorization-keys for you to test. Following the Setup Instructions in the private repo, you will use the following url to install the plugin:

https://github.com/transistorsoft/cordova-background-geolocation.git#authorization-keys
doubliez commented 3 years ago

Hi @christocracy, I tried the branch but still see the same behavior, it keeps reusing the refresh token I provide initially.

What is your exact logic? Do you first try regex on the keys, and if you don't find a match you try the other regexes on the values? Or the other way around?

BTW I think your regexes should be:

Otherwise the ^ only applies to the first alternative.

christocracy commented 3 years ago

Can you send me a complete json response of a token refresh with example data. I have a test-case I can run it through.

doubliez commented 3 years ago

Sure:

{
    "access_token": "or8RbUz5j-exEsoYVszd3JoPFkXWKqlsqnJjb5uPg08.1ub9C-RQNf6dZyfjF4J18gCMkp-xTT-C8eVfKr6SYmo",
    "expires_in": 3600,
    "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
    "refresh_token": "Qaav53eHspDeJCBEn-gkkd-CDtN0cKBTxgLtvpEvYfs.-wx13efPcxqoneIV_s_2wXhcAKTMgf5NyjtxL_3oZ2w",
    "scope": "openid offline_access",
    "token_type": "bearer"
}
christocracy commented 3 years ago

I pushed another commit to branch #authorization-keys. Uninstall / re-install the plugin from the branch and test again.

doubliez commented 3 years ago

Hi @christocracy, I tested the updated branch and now it's working as expected, thanks.

When do you plan to release this change?

christocracy commented 3 years ago

A release in the private repo is nothing more than a merge to master and a git tag. You will continue to use the plugin installed from the private repo.

The private repo will not be merged here to the public repo and released to npm for several months.

christocracy commented 3 years ago

Did you test both iOS and Android?

doubliez commented 3 years ago

I mainly tested Android.

I did some testing on iOS just now but I can't confirm if it works or not yet. My testing on Android was to terminate the app and then look at the requests on our backend (and this is working fine now as mentioned before), but on iOS it appears the plugin stops sending requests when the app is terminated. I was testing in iOS simulator with fake location set to "Freeway drive". According to the docs there is a 200m radius before it starts tracking again in this case but even when moving far out it didn't resume the requests to our backend.

So I had to test with the app running instead of terminated, but in this case the app is also performing the token refresh (in our own JavaScript code) and I need to make sure to supply the new access and refresh tokens to the plugin using BackgroundGeolocation.setConfig, otherwise the plugin will try to do a refresh on its own using a stale token.

I will do more testing next week.

christocracy commented 3 years ago

In simulator, with freeway drive, the location merely needs to change by about 200 meters before tracking resumes.

I cannot tag a release until iOS is confirmed working.

christocracy commented 2 years ago

Hi, can you please confirm iOS is working?

doubliez commented 2 years ago

@christocracy Sorry for the delay, I was able to do a proper test on iOS now by making sure our own token refresh logic does not conflict with the one done by the plugin.

So I tested the authorization-keys branch like before and it does not work. I'm logging request payloads server-side and I can see that the first token refresh works fine, but subsequent refresh requests are sent without a refresh token and therefore the OAuth2 server returns a 400 status code.

Since it was a while since my last testing, I re-tested Android and confirmed that it's working fine there.

christocracy commented 2 years ago

I pushed another commit to branch #authorization-keys. Uninstall / re-install the plugin from the branch and test again.

doubliez commented 2 years ago

I re-tested iOS and it's working now 👍 , also re-tested Android and working too.

Thanks for getting that fixed.

christocracy commented 2 years ago

Good, thanks. Release coming tomorrow.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You may also mark this issue as a "discussion" and I will leave this open.

stale[bot] commented 2 years ago

Closing this issue after a prolonged period of inactivity. Fell free to reopen this issue, if this still affecting you.