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

RedisTokenStore expiration fix #1657 #1660

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@LukasVyhlidka
Copy link

commented Apr 12, 2019

No description provided.

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

The problem in this PR is backward compatibility. It has already happened that RedisTokenStore changed the data structure from list to set. This time it is changed from set to sorted set. The prefix could be changed to something little different (e.g. client_id_to_access_z:) - what do you think?
Or there can be created a bash script that could be used to transform the set data structure to sorted set.

@LukasVyhlidka LukasVyhlidka force-pushed the LukasVyhlidka:master branch from 464b698 to 1f80547 Apr 12, 2019

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

Changed the prefix based on previous comment. At our project I would like to deploy the new version without the downtime so it has to be "backward compatible"

@LukasVyhlidka LukasVyhlidka force-pushed the LukasVyhlidka:master branch 2 times, most recently from 671cc11 to 191f347 Apr 16, 2019

@OrangeDog

This comment has been minimized.

Copy link

commented Jun 17, 2019

There is still a problem with they key's expiration being set to the last-issued expiration time.
This isn't necessarily the latest expiration time, and indeed the set could already contain tokens that never expire.

@OrangeDog

This comment has been minimized.

Copy link

commented Jun 17, 2019

While we're here, there doesn't seem to be any need for the CLIENT_ID_TO_ACCESS keys, because they are a prefix of the UNAME_TO_ACCESS keys.

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

There is still a problem with they key's expiration being set to the last-issued expiration time.
This isn't necessarily the latest expiration time, and indeed the set could already contain tokens that never expire.

You're right. I left it because it was previous behavior and did not solve an issue I was experiencing. Because the cleaning is made on the sorted set (by score values), the actual expire on whole key could be probably removed, right? On the other hand, my implementation needs to be configured so the clean method is called once upon a time. In case it is not cleaned, there would be no cleaning at all.

While we're here, there doesn't seem to be any need for the CLIENT_ID_TO_ACCESS keys, because they are a prefix of the UNAME_TO_ACCESS keys.

I don't understand what you mean. Uname is username and clientId is ID of the 3rd party app that authorizes the user (without knowing password). The client_id_to_access is needed in case I want to revoke all the tokens created for this 3rd party client app - e.g. because its password leaked or something else.

@OrangeDog

This comment has been minimized.

Copy link

commented Jun 17, 2019

the actual expire on whole key could be probably removed

Only if the cleanup routine deleted the key when the set is empty (or does that happen anyway?)

Uname is username

Despite the name, it's actually the client id and the username, used for findTokensByClientIdAndUserName.

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

when the set is empty

Well, in case the empty set is problem, then yes. Is it problem?

findTokensByClientIdAndUserName

What if I want to list all the tokens without knowing the username? In case I want all the tokens only by clientId? I can probably use SCAN for that but this looks somehow more reasonable.

@OrangeDog

This comment has been minimized.

Copy link

commented Jun 17, 2019

Is it problem?

Well yes. You'd end up with thousands and thousands of keys that are just empty sets.
No. Empty sets are not stored.

can probably use SCAN for that

Exactly. You already use SCAN in the cleanup. Using it for findTokensByClientId reduces the number of keys that need to be managed.

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Well yes. You'd end up with thousands and thousands of keys that are just empty sets.

You're right.

You already use SCAN in the cleanup.

Well, the cleanup can be started asynchronously as a background job. The scanning takes some time. In case there is a request to delete all the tokens for a client, the Scan could take quite a lot of time and the user would wait and wait...

And actually it looks to me like the CLIENT_ID_TO_ACCESS is not even part of the issue I have raised. Maybe it should be solved elsewhere.

About the expire on keys. I know that you'right. But because deleting of those expires would change the contract of the implementation dramatically, I am not sure whether it is part of this PR as well. My change only solves the memory leak and I tried not to change the contract that is there currently.

But because no one from the maintainers seems to be involved, I am not sure what is going to be with the PR anyway.

@OrangeDog

This comment has been minimized.

Copy link

commented Jun 18, 2019

On further investigation, an empty set doesn't actually exist, so there's no issue there.

@LukasVyhlidka

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

On the other hand, what is actually not needed is the ACCESS_TO_REFRESH object. It is only written and deleted, never read. And the TTL (expire) is not set corretly - it is the ttl of refresh token, but should be the ttl of access token.
Going to delete whole key in this PR.

@LukasVyhlidka LukasVyhlidka force-pushed the LukasVyhlidka:master branch from 191f347 to 09eceaa Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.