perf: move remember login tokens out of oc_preferences

[cristianscheid]

• Resolves: Move remember me token out of oc_preferences #61728

Summary

Currently, when logging in with "Remember me" selected, a token is stored in oc_preferences table:

MariaDB [nextcloud]> SELECT * FROM oc_preferences WHERE appId="login_token";
+--------+-------------+----------------------------------+-------------+------+------+-------+---------+
| userid | appid       | configkey                        | configvalue | lazy | type | flags | indexed |
+--------+-------------+----------------------------------+-------------+------+------+-------+---------+
| john   | login_token | eqqbZqE6U1t7GsYyd/hCNtMQdGXDq1k0 | 1789069718  |    0 |    0 |     0 |         |
+--------+-------------+----------------------------------+-------------+------+------+-------+---------+
1 row in set (0.001 sec)

Changes introduced by this PR:

• new dedicated table for this purporse: oc_remember_login_tokens

MariaDB [nextcloud]> SELECT * FROM oc_remember_login_tokens;
+----+------+----------------------+------------+
| id | uid  | token                | created    |
+----+------+----------------------+------------+
|  1 | john | <hashed-token-here>  | 1789070212 |
+----+------+----------------------+------------+
1 row in set (0.000 sec)

• mechanism to migrate old remember login tokens from oc_preferences to oc_remember_login_tokens
  • first look for token on oc_remember_login_tokens, if not there, look at oc_preferences
  • if found in oc_preferences, insert token on oc_remember_login_tokens and remove from oc_preferences
  • old tokens from oc_preferences should all be eventually removed either by mechanism above or by OC\User\BackgroundJobs\CleanupLoginTokens
  • OC\User\BackgroundJobs\CleanupLoginTokens cleans stale tokens, by default the ones created > 15 days

Note 1

While brainstorming how to implement this, one suggested approach was to use OCP\Security\ICredentialsManager to store the tokens.

• credentials are stored in oc_storages_credentials (columns: id, user, identifier, credentials)
• credentials column value is encrypted like $this->crypto->encrypt(json_encode($credentials))
• to store the timestamp, as is currently done in oc_preferences, the token would need to be set as the identifier (to be able to search by token), with credentials holding the timestamp:

$this->credentialsManager->store($userId, $token, $timestamp);

• this would store the token in plain text. If we wanted to hash it instead (e.g. hash('sha512', $token)), the 128-character string would be too large for the column, since identifier is defined as:

$table->addColumn('identifier', Types::STRING, [
    'notnull' => true,
    'length' => 64,
]);

• even without hash, there's still OC\User\BackgroundJobs\CleanupLoginTokens, which removes all login tokens older than a certain threshold. Since credentials column value is encrypted before being stored, cleanup would require decrypting the timestamp for each one just to determine whether an entry is stale

For 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 core

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI

[CarlSchwan]

After this get merged in master, existing dev instances will need to apply new migration to avoid table not found error:

Bump the version in version.php

[come-nc]

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.

[come-nc]

Also it would be better to switch to the new entity system.
See #63288 for an example.

[cristianscheid]

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.

I think we cannot have both things together:

• if we rotate the token by updating the entry, same snowflake ID would be kept, meaning timestamp would still hold creation time
• CleanupLoginTokens cron job would eventually cleanup this entry from DB, even if it was updated, since timestamp is still from creation time

So I think we have three options:

option 1

• use snowflake ID and use it's own timestamp to check when token was generated instead of created column
• delete + insert entry when rotating token, to generate correct timestamp

option 2

• use regular numeric ID + dedicated timestamp column
• update entry instead of delete + insert when rotating token

option 3

• use snowflake ID + dedicated timestamp column
• update entry instead of delete + insert when rotating token

cc @come-nc

[come-nc]

@cristianscheid Is it not possible to update the id as well?

[cristianscheid]

Bump the version in version.php

@CarlSchwan Thanks for the tip! Should the commit below be enough?
commit: d7aa083

[cristianscheid]

@cristianscheid Is it not possible to update the id as well?

@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 EntityManager::update().

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

There's one caveat with this approach: when migrating from oc_preferences, the old token's creation timestamp gets reset, since the row is recreated in the new table with snowflake id, encoding current timestamp instead of the original one:
SnowflakeGenerator::nextId()

edit: caveat above (strikethrough text) isn't actually a problem. Before this change, token was already regenerated at that same point in loginWithCookie(), which produces a fresh timestamp.

[provokateurin]

Since this can be null, you need to guard the usages within this class.

[cristianscheid]

Looking at the codebase, seems that OC\User\Session is only instantiated directly via new class on lib/private/Server:

	// Token providers might require a working database. This code
	// might however be called when Nextcloud is not yet setup.
	if (\OCP\Server::get(SystemConfig::class)->getValue('installed', false)) {
		$provider = $c->get(IProvider::class);
	} else {
		$provider = null;
	}

	$userSession = new Session(
		$manager,
		$session,
		$timeFactory,
		$provider,
		$c->get(IConfig::class),
		$c->get(ISecureRandom::class),
		$c->get(ILockdownManager::class),
		$c->get(LoggerInterface::class),
		$c->get(IEventDispatcher::class),
	);

Just like OC\Authentication\Token\IProvider, also present on Session's construct, if I don't define RememberLoginTokenMapper as nullable, it fails the tests since they are only available after installed.

Since it's only instantiated with null at that place, I kept the same pattern as tokenProvider, which isn't guarded either.

Given context above, do you still think it's worth adding the guard?