Feat/call check for revocation #1372

Crash-- · 2023-07-20T13:04:40Z

When we got an error for OAuth token, we check if it's something that can be used for revokation. If yes, then we make a call to know if the client is revocked or not.

BREAKING CHANGE: checkForRevocation() is only callable when dealing with an OAuthClient. It should not break your app, since if you called this method on a stackClient before, it should have crashed your app.

Crash-- · 2023-07-24T08:45:00Z

@acezard I would have add a test for the scenario:

fetch throw an error
we call refreshToken
refreshToken throw an error
we call checkForRevocation

In some cases the promise rejection is not handled at all by refreshToken, since it was not really clear what was happening it seemed safer to add an early check

packages/cozy-stack-client/src/CozyStackClient.spec.js

acezard · 2023-07-25T12:07:36Z

@acezard I would have add a test for the scenario:

fetch throw an error

we call refreshToken

refreshToken throw an error

we call checkForRevocation

@Crash-- test added, from my side everything seems good but I'm not sure I'm receiving the expected messages on revocation (with a registered client). But it still works with any error on refreshToken, we'll always do an early check

Crash-- · 2023-07-25T12:20:48Z

packages/cozy-stack-client/src/OAuthClient.js

+      )
+        // Have to catch early to check for revocation because it seems the error is not caught by the catch below in some cases (e.g. when the client is not registered, even if it should be)
+        .catch(async e => {
+          if (await this.checkForRevocation()) {


Y a un truc que je comprends pas :

déjà ce catch pourquoi il doit être de cette forme plutôt que le catch plus bas ? 🤔

le checkForRevocation fait un fetchInformation, qui lui même throw déjà une erreur throw new NotRegisteredException(). Du coup pourquoi avoir besoin de lever cette erreur plutôt que celle déjà levée?

je n'ai pas réussi à trouver la source du problème mais sans le catch chained, certaines erreurs (dont celle qui m'intéressait pour être revoked) n'étaient tout simplement jamais catch, comme si elles étaient catch ailleurs. En chainant le catch à la promise, le problème ne s'est jamais présenté à nouveau

OK ça c'est le premier point, et le second ?

OK ça c'est le premier point, et le second ?

le second, c'est que justement si je me révoque depuis settings web, la première erreur que je reçois est un "client not found", alors qu'effectivement si je comprends bien je devrais avoir un Invalid token. Mais ce n'est pas ce que m'envoie la stack en master last commit

Pourquoi tu penses que tu devrais recevoir un invalid token? On l'a déjà reçu en premier lieu, ensuite on fait un fetchInformations() qui va vérifier si justement le client existe. De là ça va throw puis passer ici https://github.com/cozy/cozy-client/pull/1372/files#diff-7818881da36e23a808dace9ce34c92ea922a6151ef1a65b66c10667bcf3660bdR678 et donc appeler le onRevocationChange. Mais l'erreur que tu vas recevoir c'est effectivement client not found.

C'est pas logique ?

Non le corps de la réponse contenait déjà the client must be registered. Mais cozy-client se base sur la valeur du header www-authenticate pour déterminer le message de la FetchError normalement (voir https://github.com/cozy/cozy-client/blob/master/packages/cozy-stack-client/src/errors.js#L56).
En tous cas c'est ce qu'il se passe dans mes essais.

anc@desktop:~/cozy/cozy-client$ curl -v -X POST 'http://dev.192-168-1-65.nip.io:8080/auth/access_token' -H 'accept: application/json' -H 'Accept-Encoding: gzip' -H 'authorization: Bearer eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJhY2Nlc3MiLCJpYXQiOjE2OTAyOTAwNDUsImlzcyI6ImRldi4xOTItMTY4LTEtNjUubmlwLmlvOjgwODAiLCJzdWIiOiJlZDczY2E0Njc0MmZhNjliMWM2ZDAzZDI4MDUxY2U4MyIsInNjb3BlIjoiKiJ9.0s-LzKsemSlJkPllcNyw0QfZ_ve2ux9xKcsQaH2S8aQv8QFP0InJTf1GRmGZok7iUdBUJSEgeyPnoXNLIrsPRw' -H 'Connection: Keep-Alive' -H 'Content-Length: 419' -H 'Content-Type: application/x-www-form-urlencoded' -H 'Cookie: _csrf=JmInvLRunjQDXYKLSFWSejcXXoBKEmlY; cozysessid=AAAAAGS_xAJlZDczY2E0Njc0MmZhNjliMWM2ZDAzZDI4MDUxNGM1YQ-b1cz6d-rpHyWKDLvo0iP15IKfE5UI7G1D7nhTC6qO' -H 'Host: dev.192-168-1-65.nip.io:8080' -H 'User-Agent: io.cozy.flagship.mobile-1.1.10' -d 'grant_type=refresh_token&refresh_token=eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZWZyZXNoIiwiaWF0IjoxNjkwMjkwMDQ1LCJpc3MiOiJkZXYuMTkyLTE2OC0xLTY1Lm5pcC5pbzo4MDgwIiwic3ViIjoiZWQ3M2NhNDY3NDJmYTY5YjFjNmQwM2QyODA1MWNlODMiLCJzY29wZSI6IioifQ.U7dTHg385sJyTiSBVICWHWNEJX5M0mWFgC5a_ye8ZrYWSEb_SIaH5rBzgndrTPADYIc-dXK40pJokamTj7VUoA&client_id=ed73ca46742fa69b1c6d03d28051ce83&client_secret=83ynXwyla94mWHtYiSzP-sFia5FPZ2nS' Note: Unnecessary use of -X or --request, POST is already inferred. * Trying 192.168.1.65:8080... * Connected to dev.192-168-1-65.nip.io (192.168.1.65) port 8080 (#0) > POST /auth/access_token HTTP/1.1 > Host: dev.192-168-1-65.nip.io:8080 > accept: application/json > Accept-Encoding: gzip > authorization: Bearer eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJhY2Nlc3MiLCJpYXQiOjE2OTAyOTAwNDUsImlzcyI6ImRldi4xOTItMTY4LTEtNjUubmlwLmlvOjgwODAiLCJzdWIiOiJlZDczY2E0Njc0MmZhNjliMWM2ZDAzZDI4MDUxY2U4MyIsInNjb3BlIjoiKiJ9.0s-LzKsemSlJkPllcNyw0QfZ_ve2ux9xKcsQaH2S8aQv8QFP0InJTf1GRmGZok7iUdBUJSEgeyPnoXNLIrsPRw > Connection: Keep-Alive > Content-Length: 419 > Content-Type: application/x-www-form-urlencoded > Cookie: _csrf=JmInvLRunjQDXYKLSFWSejcXXoBKEmlY; cozysessid=AAAAAGS_xAJlZDczY2E0Njc0MmZhNjliMWM2ZDAzZDI4MDUxNGM1YQ-b1cz6d-rpHyWKDLvo0iP15IKfE5UI7G1D7nhTC6qO > User-Agent: io.cozy.flagship.mobile-1.1.10 > * Mark bundle as not supporting multiuse < HTTP/1.1 400 Bad Request < Content-Type: application/json; charset=UTF-8 < Date: Tue, 25 Jul 2023 13:09:16 GMT < Content-Length: 42 < {"error":"the client must be registered"} * Connection #0 to host dev.192-168-1-65.nip.io left intact

curl de mon POST access_token pour ref. Je n'ai pas l'impression qu'il y ait le header www-authenticate

par contre je l'ai bien à la requête qui précède (donc celle qui trigger le refreshToken)

Pour info, dans mes tests, Desktop faisait une requête sur /settings/disk-usage.
C'est cette requête qui recevait une erreur Expired Token déclenchant un refresh.

Je viens de tester à nouveau et effectivement cozy-client ne voit cette erreur ni comme un Expired Token ni comme un Invalid Token parce qu'elle utilise le corps de la réponse comme reason.

Ldoppea · 2023-07-25T13:06:27Z

@acezard I'm wondering if it would be easier to read the PR with integration tests that mock only the fetch returns

I did that in another PR and it has the advantage of making easier to vizualize the different scenario and what are the cozy-stack answers:
https://github.com/cozy/cozy-client/pull/1352/files#diff-a5c3691d6cd920e73de39e425c774e7589a6fb926621b2b9718cb7c2b9ebccb9R426-R450

acezard · 2023-07-25T13:08:28Z

@acezard I'm wondering if it would be easier to read the PR with integration tests that mock only the fetch returns

I did that in another PR and it has the advantage of making easier to vizualize the different scenario and what are the cozy-stack answers: https://github.com/cozy/cozy-client/pull/1352/files#diff-a5c3691d6cd920e73de39e425c774e7589a6fb926621b2b9718cb7c2b9ebccb9R426-R450

I agree on principle, but from my tests in runtime, the stack didn't send what I expected. Thus, I preferred to assume the end result of the calls, not the stack answers. But I agree that I don't like mocking internal methods in a test

packages/cozy-client/src/CozyClient.js

Crash-- · 2023-07-26T07:04:57Z

packages/cozy-stack-client/src/OAuthClient.js

-    })
+      return newToken
+    } catch (e) {
+      if (errors.UNREGISTERED_CLIENT.test(e.message)) {


Why not using this.isRevocationError ?

Yes but then we need to update it because currently it only tests for const CLIENT_NOT_FOUND = /Client not found/. I'm on it

Updated

Crash-- · 2023-07-26T07:05:56Z

packages/cozy-stack-client/src/OAuthClient.js

-    this.setToken(newToken)
+        throw new Error(
+          'Client has been revoked or does not exist. Please authenticate again.'
+        )


I don't know if we should change the error message. Shouldn't we throw the real error? Or a fetch error?

Well yes I guess I wanted to imitate the checkForRevocation conditional

const revoked = await this.checkForRevocation() if (revoked) { throw new Error('Client has been revoked. Please authenticate again.') }

I'm not sure if it's still a fetch error at this point, it's more a reaction to a fetch error. But we could indeed at least keep the error message and add more info saying that error message probably happens because client was either revoked or doesn't exist (both can be true)

Crash-- · 2023-07-26T07:06:58Z

packages/cozy-stack-client/src/OAuthClient.js


-    return newToken
+        if (revoked) {


same here, I think that checkForRevocation() should throw the real error.

ok, then the initial fetcherror? as I mentioned earlier, I think we should add some more context info maybe. Like adding we think the client is revoked (I mean it's 100% sure it doesn't exist, but it's either revoked or non-existing)

Crash-- · 2023-07-26T07:07:19Z

packages/cozy-stack-client/src/OAuthClient.js

+   *
+   * @async
+   * @returns {Promise<boolean>} A Promise that resolves to `false` if client is still valid, or `true` if it has been revoked.
+   * @throws {Error} If an error occurs while fetching the client information.


With this implementation, this is not true, but we should throw https://github.com/cozy/cozy-client/pull/1372/files#r1274467375

Alright, it's fine by me I think both throwing a generic error or the initial fetcherror is fine as long as some context info is added during handling of the initial error

Crash-- · 2023-07-27T07:04:31Z

packages/cozy-stack-client/src/OAuthClient.js

+   *
+   * @async
+   * @returns {Promise<boolean>} A Promise that resolves to `false` if client is still valid, or `true` if it has been revoked.
+   * @throws {FetchError} If an error occurs while fetching the client information.


This method doesn't throw

@Crash-- updated

packages/cozy-stack-client/src/OAuthClient.js

Crash-- added 2 commits July 20, 2023 15:04

feat: CheckForRevocation automatically after an invalid token

74204b9

BREAKING CHANGE: checkForRevocation() is only callable when dealing with an OAuthClient. It should not break your app, since if you called this method on a stackClient before, it should have crashed your app.

refactor: Group import

6e5b924

Crash-- requested review from Merkur39 and paultranvan as code owners July 20, 2023 13:04

acezard added 2 commits July 21, 2023 14:59

feat: Update docs for revocation changes

5d2a094

feat: Add unit test to revocation event emitter

3cc0101

acezard self-assigned this Jul 24, 2023

feat: Add Oauth test and catch early

cc4d0de

In some cases the promise rejection is not handled at all by refreshToken, since it was not really clear what was happening it seemed safer to add an early check

acezard reviewed Jul 25, 2023

View reviewed changes

packages/cozy-stack-client/src/CozyStackClient.spec.js Show resolved Hide resolved

acezard changed the title ~~[WIP] Feat/call check for revocation~~ Feat/call check for revocation Jul 25, 2023

Crash-- commented Jul 25, 2023

View reviewed changes

acezard force-pushed the feat/CallCheckForRevocation branch from f515e75 to 5e198d2 Compare July 26, 2023 07:00

Crash-- commented Jul 26, 2023

View reviewed changes

packages/cozy-client/src/CozyClient.js Show resolved Hide resolved

Crash-- commented Jul 26, 2023

View reviewed changes

acezard force-pushed the feat/CallCheckForRevocation branch 4 times, most recently from dc3bc1a to ecae5a2 Compare July 26, 2023 07:39

Crash-- commented Jul 27, 2023

View reviewed changes

cballevre reviewed Jul 27, 2023

View reviewed changes

packages/cozy-stack-client/src/OAuthClient.js Outdated Show resolved Hide resolved

acezard force-pushed the feat/CallCheckForRevocation branch 2 times, most recently from 6a56c15 to 6d9303a Compare July 27, 2023 07:37

acezard self-requested a review July 27, 2023 07:37

acezard requested review from cballevre and taratatach July 27, 2023 07:37

acezard approved these changes Jul 27, 2023

View reviewed changes

fix: Update tests for unregistered error handling

3899557

acezard force-pushed the feat/CallCheckForRevocation branch from 6d9303a to 3899557 Compare July 27, 2023 07:41

Crash-- merged commit 0e9b96b into master Jul 27, 2023
4 checks passed

Crash-- deleted the feat/CallCheckForRevocation branch July 27, 2023 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/call check for revocation #1372

Feat/call check for revocation #1372

Crash-- commented Jul 20, 2023 •

edited

Loading

Crash-- commented Jul 24, 2023

acezard commented Jul 25, 2023 •

edited

Loading

Crash-- Jul 25, 2023

acezard Jul 25, 2023

Crash-- Jul 25, 2023

acezard Jul 25, 2023

Crash-- Jul 25, 2023

taratatach Jul 25, 2023

acezard Jul 25, 2023

acezard Jul 25, 2023

taratatach Jul 25, 2023

taratatach Jul 25, 2023 •

edited

Loading

Ldoppea commented Jul 25, 2023

acezard commented Jul 25, 2023

Crash-- Jul 26, 2023

acezard Jul 26, 2023 •

edited

Loading

Crash-- Jul 26, 2023

acezard Jul 26, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading

Crash-- Jul 26, 2023

acezard Jul 26, 2023

acezard Jul 26, 2023 •

edited

Loading

Crash-- Jul 26, 2023

acezard Jul 26, 2023

acezard Jul 26, 2023 •

edited

Loading

Crash-- Jul 27, 2023

acezard Jul 27, 2023

Feat/call check for revocation #1372

Feat/call check for revocation #1372

Conversation

Crash-- commented Jul 20, 2023 • edited Loading

Crash-- commented Jul 24, 2023

acezard commented Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taratatach Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Ldoppea commented Jul 25, 2023

acezard commented Jul 25, 2023

Choose a reason for hiding this comment

acezard Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezard Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

acezard Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezard Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezard Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crash-- commented Jul 20, 2023 •

edited

Loading

acezard commented Jul 25, 2023 •

edited

Loading

taratatach Jul 25, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading

acezard Jul 26, 2023 •

edited

Loading