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

Add is_expired Method for Clients #754

Open
Borthralla opened this issue Apr 9, 2021 · 5 comments · May be fixed by #846
Open

Add is_expired Method for Clients #754

Borthralla opened this issue Apr 9, 2021 · 5 comments · May be fixed by #846
Labels
Contributor Friendly Feature OAuth2-Client This impact the client part of OAuth2.

Comments

@Borthralla
Copy link

Borthralla commented Apr 9, 2021

Currently, Oauth2 clients contain properties expires_in and _expires_at.
In the method which adds the token to a request, the fields are used liked this:

        if self._expires_at and self._expires_at < time.time():
            raise TokenExpiredError()

It would be useful to someone using the client to be able to check whether the token is expired before they try to add it to a request. Currently, in order to do that they would have to go into the code and see that it uses the time library and duplicate the above line of code.

It would be much more convenient (and more organized in the code itself) to have this be a method is_expired() which could then be exposed to users of the client.

Applies to Oauth2 Client side code.

@JonathanHuot
Copy link
Member

I suppose the method can have an additional argument verify=by default set to True but could also be a dict like:

{
"secure_protocol": True/False
"token_type": True/False,
"access_token": True/False,
"expire_at": True/False
}

A little bit alà pyJWT https://pyjwt.readthedocs.io/en/latest/usage.html#reading-the-claimset-without-validation

Thoughts?

@JonathanHuot JonathanHuot added Discussion Feature OAuth2-Client This impact the client part of OAuth2. labels Jun 18, 2021
@Borthralla
Copy link
Author

Borthralla commented Jun 23, 2021

I suppose the method can have an additional argument verify=by default set to True but could also be a dict like:

{
"secure_protocol": True/False
"token_type": True/False,
"access_token": True/False,
"expire_at": True/False
}

A little bit alà pyJWT https://pyjwt.readthedocs.io/en/latest/usage.html#reading-the-claimset-without-validation

Thoughts?

That doesn't seem necessary to me, the existing add_token method is fine as it is. I don't think there's a good reason not to check the expiration time when adding the token.
What I'm proposing is simply making the expiration check a regular method that is available to someone using the library. The advantage of that approach is that the expiration check can be done directly without the try/catch.
For example instead of this kind of code


try:
    client.add_token(url)
except TokenExpired:
    authenticate(client)
    client.add_token(url)

It might look like this

if client.is_expired():
    authenticate(client)
client.add_token(url)

Which seems a bit cleaner to me.

@JonathanHuot
Copy link
Member

The problem with this approach is twofold:

  • It becomes incompatible with previous interface
  • Using Exception to return different states of a calling functions, is more a style question than something else, but that should be applied to the project as a whole. Since oauthlib has started to use Exception mechanisms to return these states, I'm not sure it's worth to change it now.

Any Thoughts?

@Borthralla
Copy link
Author

Borthralla commented Aug 11, 2021

The problem with this approach is twofold:

  • It becomes incompatible with previous interface
  • Using Exception to return different states of a calling functions, is more a style question than something else, but that should be applied to the project as a whole. Since oauthlib has started to use Exception mechanisms to return these states, I'm not sure it's worth to change it now.

Any Thoughts?

I'm not arguing for moving everybody to that approach, the same check will occur with the same exception so I'm not sure how the interface is different. I'm just requesting that the check be put in a visible method in case someone wanted to check it themselves.

@JonathanHuot
Copy link
Member

Ah sure, then I don't have objections, it seems a reasonable PR :)

wilbertom added a commit to wilbertom/oauthlib that referenced this issue Apr 5, 2023
@wilbertom wilbertom linked a pull request Apr 5, 2023 that will close this issue
wilbertom added a commit to wilbertom/oauthlib that referenced this issue Aug 24, 2023
wilbertom added a commit to wilbertom/oauthlib that referenced this issue Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor Friendly Feature OAuth2-Client This impact the client part of OAuth2.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants