Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

token storage #10

Closed
vladislavs1321 opened this issue Mar 22, 2018 · 11 comments
Closed

token storage #10

vladislavs1321 opened this issue Mar 22, 2018 · 11 comments

Comments

@vladislavs1321
Copy link

vladislavs1321 commented Mar 22, 2018

is it possible to have default token storage/cache to prevent each api call producing new token?

@gregurco
Copy link
Owner

@vladislavs1321 good proposal 👍 gonna to do it as soon as will have time

@vladislavs1321
Copy link
Author

@gregurco, sorry for disturbing. Any news? Quite sensitive problem for me)

@gregurco
Copy link
Owner

gregurco commented Apr 4, 2018

@vladislavs1321 hello. I'm thinking about this problem. I have some ideas and let me try to do a concept today/tomorrow. I will write here the result.

@gregurco
Copy link
Owner

gregurco commented Apr 4, 2018

@vladislavs1321 I did changes on branch https://github.com/gregurco/GuzzleBundleOAuth2Plugin/tree/persistent_storage .
Please do composer require gregurco/guzzle-bundle-oauth2-plugin dev-persistent_storage
and setup plugin to be "persistent":

eight_points_guzzle:
    clients:
        your_client_name:
            # ...
            
            plugin:
                oauth2:
                    # your configuration
                    persistent:  true

So, in this case access token will be persisted in session (not token storage) and used in future requests... Please write feedback after testing if you are able to test to know if it's ok and ready to merge and release.

@vladislavs1321
Copy link
Author

@gregurco , for the 1st glance looks good, but i've found some unhandled cases:

  1. if token stored on client, but no more exist on server, there is 401 Unauthorized : The access token provided is invalid. I think in this case we need to try to fetch new one.
  2. Also maybe make sense to somehow work with lifetime ?

But anyway - good job, thanks a lot!

@gregurco
Copy link
Owner

gregurco commented Apr 5, 2018

@vladislavs1321 thanks for remarks :) yep, I didn't check this case. But lifetime is optional and yep, we can check it but better to retry to get the token once again... I will try to fix it today

@gregurco
Copy link
Owner

gregurco commented Apr 5, 2018

@vladislavs1321
https://github.com/Sainsburys/guzzle-oauth2-plugin/blob/master/src/Middleware/OAuthMiddleware.php#L127 - here you can see, that if token is expired it will be updated right before request. So, the case with expired token restored from session is good handled by default middleware logic.

https://github.com/Sainsburys/guzzle-oauth2-plugin/blob/master/src/Middleware/OAuthMiddleware.php#L100 - here you can see the case when we have token, it has no expiration date or expiration date is not valid (it's in future but token it not valid already) and yep, middleware will not do force update. And for me it's ok, because with such a logic you can enter in infinite loop. So, for now my suggestion is to use persistent option with trustable expiration date.

Also there remains possibility to delete token from session manually:

$container->get('session')->remove('yourClientName_token');

right before doing request. And it will not be restored from session and will be requested new token.

Any ideas?

@vladislavs1321
Copy link
Author

vladislavs1321 commented Apr 5, 2018

@gregurco Hmm, then maybe proper way will by throw special error, coz we already change default workflow?
It will indicate, that something not valid, and also shows, that we are using persistent option?
In this case possible to handle this in future with listener in application or at least better understand what happens.
UPD:
I've found that even with a complete down infrastructure (my local development environment based on docker), token remains in session, and then starts to call api endpoints with this remained token, that not exist in your system at all, for example if you reset your database with new set of fixtures.

@gregurco
Copy link
Owner

@vladislavs1321 I think it's better to split it for now. I think it's better to merge current implementation and to do an release and after that step by step to handle different cases. What do you think?

@vladislavs1321
Copy link
Author

@gregurco im ok with this, let's collect feedbacks)

@gregurco
Copy link
Owner

@vladislavs1321 thanks :) I created and merged PR #11 . In few minutes I will do a release and I will create another issue for additional functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants