perf: move remember login tokens out of oc_preferences - #64203
perf: move remember login tokens out of oc_preferences#64203cristianscheid wants to merge 3 commits into
Conversation
2968e13 to
34462cd
Compare
Bump the version in version.php |
come-nc
left a comment
There was a problem hiding this comment.
I think it would make sense to update the entity when rotating the token, rather than deleting+inserting.
I’m also wondering whether we could use snowflake ids instead of numeric id + created at. The snowflake contains the creation timestamp.
|
Also it would be better to switch to the new entity system. |
I think we cannot have both things together:
So I think we have three options: option 1
option 2
option 3
cc @come-nc |
|
@cristianscheid Is it not possible to update the id as well? |
Signed-off-by: Cristian Scheid <cristianscheid@gmail.com>
Signed-off-by: Cristian Scheid <cristianscheid@gmail.com>
34462cd to
7eade0a
Compare
@CarlSchwan Thanks for the tip! Should the commit below be enough? |
@come-nc I did not consider this approach at first since EntityManager::update() does not allow updating ID, but looking at codebase again I think we can follow a similar approach to AccessTokenMapper::rotateToken(), which does a more direct update using query builder instead of We could do something similar to update both the token and the snowflake ID, refreshing it's timestamp. I implemented this on this commit: 7eade0a
|
7eade0a to
2fd0a09
Compare
| $table->addUniqueIndex(['token'], 'remember_login_tokens_token'); | ||
| $table->addIndex(['uid'], 'remember_login_tokens_uid'); | ||
| // Makes sure there is no auto-increment in Oracle | ||
| $schema->dropAutoincrementColumn('remember_login_tokens', 'id'); |
There was a problem hiding this comment.
added this here since I saw same approach on 'job_runs' table, which also uses snowflake IDs:
There was a problem hiding this comment.
technically not needed as this was never merged as a autoincremented column but this also doesn't hurt
There was a problem hiding this comment.
you think it's safe to remove the dropAutoincrementColumn() then?
…ake ids Signed-off-by: Cristian Scheid <cristianscheid@gmail.com>
2fd0a09 to
41d9870
Compare
oc_preferences#61728Summary
Currently, when logging in with "Remember me" selected, a token is stored in
oc_preferencestable:Changes introduced by this PR:
oc_remember_login_tokensoc_preferencestooc_remember_login_tokensoc_remember_login_tokens, if not there, look atoc_preferencesoc_preferences, insert token onoc_remember_login_tokensand remove fromoc_preferencesoc_preferencesshould all be eventually removed either by mechanism above or byOC\User\BackgroundJobs\CleanupLoginTokensOC\User\BackgroundJobs\CleanupLoginTokenscleans stale tokens, by default the ones created > 15 daysNote 1
While brainstorming how to implement this, one suggested approach was to use
OCP\Security\ICredentialsManagerto store the tokens.oc_storages_credentials(columns:id, user, identifier, credentials)credentialscolumn value is encrypted like$this->crypto->encrypt(json_encode($credentials))oc_preferences, the token would need to be set as theidentifier(to be able to search by token), withcredentialsholding the timestamp:hash('sha512', $token)), the 128-character string would be too large for the column, sinceidentifieris defined as:OC\User\BackgroundJobs\CleanupLoginTokens, which removes all login tokens older than a certain threshold. Sincecredentialscolumn value is encrypted before being stored, cleanup would require decrypting the timestamp for each one just to determine whether an entry is staleFor these reasons, I went with a dedicated table instead, since we can store the hashed value of the token, and easily delete stale records with a direct query, since the timestamp itself is not hashed.
Note 2
After this get merged in master, existing dev instances will need to apply new migration to avoid table not found error:
# to run pending migrations for core occ migrations:migrate coreChecklist
3. to review, feature component)stable32)AI (if applicable)