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

Need some form of notification when pycrest acquires a new access token from the refresh token #46

Open
wtfrank opened this issue Oct 7, 2016 · 6 comments

Comments

@wtfrank
Copy link
Contributor

wtfrank commented Oct 7, 2016

When we initialise a pycrest object we pass in the long-lived refresh token which pycrest uses to get a short-lived access token via web request.

While this access token is still valid, it would be desirable to retain this token and avoid some situations where you make another call to obtain a short-lived access token before the access token had expired.

We know when the access token will expire, so if we triggered some kind of event when a new access token was acquired, we could store this along with the expiry time, then when initialising a future pycrest object we could save a web request if the old access token had not expired.

@wtfrank wtfrank changed the title Need some form of notification when pycrest triggers a refresh on its access token Need some form of notification when pycrest acquires a new access token from the refresh token Oct 7, 2016
@hkraal
Copy link
Contributor

hkraal commented Oct 8, 2016

I'm probably missing your point but why would you care about a short lived token expiry when you got the long lived token which will be valid forever? If the short lived token has been expired on the authed instance it gets automaticly refreshed (https://github.com/pycrest/PyCrest/blob/master/pycrest/eve.py#L425) and as such you don't need to worry about it's expiry?

@wtfrank
Copy link
Contributor Author

wtfrank commented Oct 21, 2016

the short lived token lasts 20 minutes. lets say you fire up a process that uses pycrest, it gets a short lived token and uses it, then the process terminates. But 5 minutes later you fire up another pycrest process, so this again performs the http call to get a short-lived token even though the previous one was still valid for another 15m. It would have been more efficient to have retained the short-lived acess token so that it could be reused if its still valid, without having to request a new one.

Hence, the idea to trigger some form of event when its updated, so that the application uses pycrest has the opportunity to store it if it chooses.

@Kyria
Copy link
Contributor

Kyria commented Oct 21, 2016

I think it really depends on how you use pycrest / login to crest, thus it might not be that useful for everyone. But It could be nice to have pycrest sending a signal when it updates the token (in case we want to store it somewhere, like in session/cookies), even if by default this signal is ignored or linked to a dummy signal handler.

At least, and if someone needs it, everything exists and is ready to use. The user will just have to make his handler and give it to PyCrest to overwrite a default "i_do_nothing_handler", and enjoy it.

@Kyria
Copy link
Contributor

Kyria commented Nov 24, 2016

I was thinking about it, as I want to put this in the ESI version i'm currently (trying to) doing.

I think the easiest way to implement this, is to use an Observer pattern (reminder). So we'll only need 2 (3 actually) things :

  1. A way to append/set an observer to a current list of observers
  2. Add the observer.notify() when you refresh the token (basically, it'll be something like this in the refresh method :
for observer in self.observer_list:
  observer.notify_update(
    access_token=new_token,
    refresh_token=new_refresh,
    expire=new_expire
  )
  1. (only to prevent error) Define a BaseObserver to check observers when added (the same way we have APICache)

What do you think about this ?

@wtfrank
Copy link
Contributor Author

wtfrank commented Nov 24, 2016

Yeah observer pattern was always the standard way to do this back when I did C++. I've noticed that django and celery have some kind of event system where you use a decorator to specify that a function receives a signal (https://docs.djangoproject.com/en/1.10/topics/signals/). Presumably this is implemented with that pattern in the background, but I've been meaning to poke around in django and see how they do this, maybe there are some python specific tricks.

@Kyria
Copy link
Contributor

Kyria commented Nov 24, 2016

Yep, that's what they do (more info if you want).
I don't think we need something as big/elaborate/complicated/[put your words] as what django do, as long as it does the job the way people will expect it to work.

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

No branches or pull requests

3 participants