allow creation of multiple access tokens per client id

[DeepDiver1975]

fixes #64

[codecov]

Codecov Report

Merging #65 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #65      +/-   ##
============================================
+ Coverage     83.93%   83.96%   +0.02%     
  Complexity      180      180              
============================================
  Files            20       20              
  Lines           635      636       +1     
============================================
+ Hits            533      534       +1     
  Misses          102      102

Impacted Files	Coverage Δ	Complexity Δ
lib/Controller/PageController.php	65.97% <ø> (-1.03%)	29 <0> (ø)
lib/Controller/OAuthApiController.php	100% <100%> (ø)	18 <0> (ø)	⬇️
lib/Db/RefreshToken.php	100% <100%> (ø)	1 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 498ef65...9e73eac. Read the comment docs.

[joneug]

@DeepDiver1975 Please also have a look at the OAuthApiController. I think it is not enough to just remove the three lines.

One has to think of the following:

• I think it is no problem to issue multiple authorization codes. If a client doesn't use one it will expire after 10 minutes and will be deleted by the background job.
• With a valid authorization code an access token can be requested. Again, there is no problem in having multiple access tokens per client. Together with these access tokens there are refresh tokens issued.
• With a valid refresh token an access token can be requested, too. And I think we have to be careful here. We have to delete the old access token that is refreshed using the refresh token. In the current implementation it is simply done by deleting all access tokens and refresh tokens and issuing a completely new pair. But when allowing multiple access tokens we need to change the model to remember the connection between an access token and its refresh token. If this access token gets refreshed we can delete both and issue a new pair. We also have to remember that an access token could already be deleted when getting refreshed with a refresh token.

Does that make sense?

[DeepDiver1975]

Does that make sense?

Absolutly! Thanks a lot for pointing this out.

Lets add a relationship between refresh token and access token - similar to https://github.com/thephpleague/oauth2-server/blob/master/src/Entities/RefreshTokenEntityInterface.php#L42-L54

[ogoffart]

I tested the use case of the client with several time the same account (which should not really be supported use case), and it works fine 👍 .

The surprising thing is that even restarting the client works fine despite the fact that, as I mention in owncloud/client#5830 (comment) , the client only store one refresh_token in the keychain. Meaning that the refresh_token can be re-used several times. Is that expected that one can re-use the refresh_token ?

[DeepDiver1975]

Is that expected that one can re-use the refresh_token ?

the refresh token is deleted after use - see https://github.com/owncloud/oauth2/pull/65/files#diff-db452df3926cad18dfda329e7e3fddf4R176

something not working properly here?

[SamuAlfageme]

Is that expected that one can re-use the refresh_token ?

I'd say no, they have to maintain a 1-1 rel. with the auth_token

The refresh_token shouldn't be universal for an account... Imagine the scenario where we want session-level management (i.e. what GMail/Facebook do; there might be many sessions opened by the same user in the same clients but in different locations/devices); we should be able to revoke both tokens at a time.

[ogoffart]

The client does two POST to the token endpoint at the same time, with the same refresh_token. Both succeeds and return different new access_token and refresh_token.
Maybe there is a race condition?

[DeepDiver1975]

The client does two POST to the token endpoint at the same time,

can you test this with consecutive calls?

[SamuAlfageme]

Both succeeds and return different new access_token and refresh_token.

I run the scenario quite some times now and I saw that reproduced a few of them. So the race is to be considered, yes.

can you test this with consecutive calls?

And those times the requests don't reach the server that simultaneously, the first POST succeeds; second is replied:

{
    "error": "invalid_grant"
}

... and the client pops-up a new browser to Authorize the app again. So after all, we might need to adapt client-side for the edge-case in owncloud/client#5830 😕 (i.e. one of the logged-in accounts is left unauthorized upon client restart).

Also, if we space in time the POST requests to https://<server>/index.php/apps/oauth2/api/v1/token wouldn't the second one use the access token the first just got... and succeed (invalidating & overwriting the one just used)?

That will never happen in the scenario of 2 independent clients sharing the same id/secret with the same account connected as it's inherent to this refresh-token-shared-between-accounts issue.

cc/ @ogoffart @DeepDiver1975

[DeepDiver1975]

let me rebase this ....

[SamuAlfageme]

Concurrency issues moved to #70 - apart from that, rest of the PR is 👍