-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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 expiration of unused refresh tokens #108428
Add expiration of unused refresh tokens #108428
Conversation
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
881107b
to
2ae8e36
Compare
2ae8e36
to
c2aecb5
Compare
rebased on latest |
meehhh ... one test failure is gone, a new one raises 🙄 🙈 |
664b887
to
6e519b6
Compare
I discussed this yesterday with some and I want to change this to apply to existing refresh tokens too. Let me add a little context. Initially, this was discussed on Discord, where the concern was raised: We don't know how existing refresh tokens are used: maybe in some applications... This resulted in the idea of only using it for new tokens. However, there is no normal way to create these tokens manually. Any token you've created in the UI for these purposes (e.g., to hook up Node-RED or anything else) are long-lived access tokens (to which this logic already doesn't apply). What is left is potentially a logged-in user, that hasn't used their application in 3 months or more. In the, assumingly, rare case that happens, a user will be logged out and can log in again. The latter is the intended behavior of this change anyways. ../Frenck |
I agree. The majority of these tokens are never going to get cleaned up if we don't start the 3 month timer on all the old ones as well. If we wanted to be extra safe we could set the timer to be 6mo or 1yr for the old ones to give people time to sort things out if they only have access to HA one a year because they have the install in a remote location... but that seems like an extreme case. |
I'm also fine with let exiting tokens expire, too👍 |
Let's do 90 days for all. |
I'll test this in a bit |
I have one token that isn't getting an expire time... digging in |
nevermind, token type is system. This is expeted |
checking Profiler: Log event loop scheduled |
I changed the expire time to 1 minute to test locally and verified I got logged out. Now checking to make sure expire_at is being extended ok |
Running a 40 minute test now |
Testing looks great. No ill effects observed on production either |
Thanks @mib1185 |
Breaking change
Refresh tokens will be automatically deleted when unused. A refresh token is considered unused if it has not been used for a login within 90 days.
If you need a permanent token, then we recommend using Long-lived access tokens.
Proposed change
This adds an expiration period for unused refresh tokens of 90 days.
This only applies for "normal" (
TOKEN_TYPE_NORMAL
) tokens.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: