Refactor Access Tokens #2651
Refactor Access Tokens #2651
Conversation
| ->cursor(); | ||
|
|
||
| // Insert initial value for our new primary key | ||
| foreach ($tokens as $i => $token) { |
There was a problem hiding this comment.
I have to say, I'm a bit worried about this one.
Because of the garbage collector bug, some forums will have accumulated tens/hundreds of thousands/millions? if not more access tokens.
Should I include the equivalent of the garbage collector in there to clear all the expired tokens before the migration?
My thought was that the migration would be simpler by not going the garbage deletion, since it will happen when they start browsing the updated forum anyway.
I'm not sure if anyone has a large forum backup we could try this on.
There was a problem hiding this comment.
I'm not sure if anyone has a large forum backup we could try this on.
My forum has 48k access tokens and I also have a local installation based on a recent backup, if necessary
There was a problem hiding this comment.
Thanks for the info!
I have modified the migrations, the new version should actually delete most of the tokens before attempting to update those that are remaining (only remember tokens will be kept).
Do you know how to run the dev-master of Flarum? That would be a great test once we merge.
I don't want to ask for a copy of the table since it's obviously sensitive data.
There was a problem hiding this comment.
Do you know how to run the dev-master of Flarum? That would be a great test once we merge.
To be honest, last time I tried I didn't succeed, because of some Composer constraints issues IIRC. But I can try again. You know where to find me :)
There was a problem hiding this comment.
@matteocontrini actually, if you could send me a database dump of just the access_tokens table, but without the token column, that would be great! I think some tools like phpMyAdmin allow selecting columns during export.
I'm pretty sure I have a discussion with you on Discord, but I can't recall your username 🙈
|
I have pushed an update. Since most tokens in the table actually won't be used anymore, even if they are still technically valid, I might as well delete them. So now the following happens on update:
I have also fixed the remember from cookie code, and added tests for it. When running the tests locally I get a bunch of the following error from other tests: I also see the tests stopped running in our CI so we'll probably need to wait on that to recover so I can see if the error is just for me or not. |
|
|
||
| // Insert initial value for our new primary key | ||
| // This migration runs after the "add type" migration, this ensures we have a minimal number of tokens remaining | ||
| foreach ($tokens as $i => $token) { |
There was a problem hiding this comment.
How long did this take in your environment? I don't know if there's a way to set all these with one query, but even with clearing out the vast majority of tokens, this could be thousands of queries on larger environments
| * This value will be used in the validity and expiration checks. | ||
| * @var int Lifetime in seconds. Zero means it will never expire. | ||
| */ | ||
| protected static $lifetime = 0; |
There was a problem hiding this comment.
Shouldn't the default token type have a non-infinite lifetime?
There was a problem hiding this comment.
That's up for debate. I had no particular preference.
There was a problem hiding this comment.
Could we make the base type an abstract class so it can't be instantiated? Then it wouldn't matter. This might cause issues with the retrieval logic, I'm not sure if Laravel would automatically convert it to the proper type. It'd also require that if extensions want to introduce new types, they do so properly.
There was a problem hiding this comment.
I just tested, we cannot make it abstract. It doesn't work with Eloquent.
Again, we have the same situation with Post where the base post isn't abstract.
I guess the question here is whether we prefer 0 as the default, or 3600 ? I don't think any other value makes sense.
There was a problem hiding this comment.
Can we set it to null (or not have it at all) in the base class?
There was a problem hiding this comment.
Personally this seems even more confusing. Despite the lack of parameter type-hinting (which I see came in PHP 7.4 or 8?), having a number there makes it obvious what you should set.
If it's null and someone doesn't override it, then I don't think there will be any error, but it also won't behave differently from 0 since it's used in multiple math operations.
| $this->setAccessTokenTypes(); | ||
| } | ||
|
|
||
| protected function setAccessTokenTypes() |
There was a problem hiding this comment.
Let's make this a container binding in register; the difference for this PR wouldn't be that significant, and we'd need this anyway later for the extender.
There was a problem hiding this comment.
The type array I mean; the static assignment should be where it is now. I'd even argue to put it directly in boot.
There was a problem hiding this comment.
This is a copy-paste of how post types work. I don't mind changing it, but then it would be logical to also change it for post types.
Make session token-based instead of user-based Clear current session access tokens on logout Introduce increment ID so we can show tokens to moderators in the future without exposing secrets Switch to type classes to manage the different token types. New implementation fixes #2075 Drop ability to customize lifetime per-token Add developer access keys that don't expire. These must be created from the database for now Add title in preparation for the developer token UI Add IP and user agent logging
[ci skip] [skip ci]
Since they were not re-used after login anyway
askvortsov1
force-pushed
the
cw/access-token-refactor
branch
from
158557a
to
8d2c7dd
Compare
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Move remember parameter to /api/token and add migration layer for lifetime parameter
[ci skip] [skip ci]
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
|
New changes:
|
| * This value will be used in the validity and expiration checks. | ||
| * @var int Lifetime in seconds. Zero means it will never expire. | ||
| */ | ||
| protected static $lifetime = 0; |
There was a problem hiding this comment.
Can we set it to null (or not have it at all) in the base class?
|
I pushed some more commits and deprecated There's still the question of migration performance which I haven't really been able to test. |
|
Also, looks like tests are failing: there's one instance where it gets the order wrong. I think there should be an order-independent comparison option? |
|
I'm really not sure how I would do an order-independent check. I opted to sort them before the comparison. It seems to be a MariaDB thing? In MySQL they are always returned in the order they were inserted if no order is specified. I'm going to do a performance test on migrations shortly. We should hold up merging until then. |
|
Performance is indeed not ideal... Testing with @matteocontrini data, we start with 50k tokens, then are left with 19k remember tokens. 38% remember tokens seems like a lot, but that includes all social logins so it might make sense. On the command line with a local MySQL server it took 40 seconds. I'm not sure exactly how many database requests were made in the end, but MySQL workbench showed an average of 500 requests per second during that migration.
I have also checked the ratio on the Discuss database. There's around 25k tokens and we average 39% remember tokens. So that's very similar. That's more remember tokens than I expected to remain on average. It's sad because most of those tokens are probably long forgotten by any client but we have no way of knowing. That kind of relates to the new issue I created to discuss remember me lifetime. I will check if I find any better solution. Either something fully SQL based. Or moving to update batches, which will already save a lot of requests. |
|
Is it necessary to actually fill in all the IDs? If the column is added as a primary key, StackOverflow seems to suggest that it might be autopopulated: https://stackoverflow.com/questions/14753321/add-auto-increment-id-to-existing-table/17346658 |
|
Oh yeah I should have tried that earlier. I started with it but must have been confused by errors which were in fact due to Laravel trying to run the instructions in the wrong order. That's why there are multiple Now that I've moved things around a bit it works without any code to set the values. Let's hope it works on MariaDB as well 😅 I don't have an easy access to MadiaDB to test. It's down to around 4 seconds with the current solution, and I didn't see any spike in MySQL resources. |
|
So... I guess we're ready for merge now? I can also confirm I tested running the installation wizard, and logging in with the FoF Oauth extension, which all worked perfectly. I also tried each of registration, password change and email change which all correctly log the user afterwards. I'll just need to move my text from the first discussion into a PR to the documentation. Not sure of the process for that since there's no beta 16 update page yet. |
This pull request contains all the breaking changes that are necessary to properly implement the security roadmap as well as for the new session management screen in #2074
Since the important breaking changes aren't really tied to the management screen, I extracted those to a separate PR so we can merge them for the next release.
The following changed:
AccessToken::find()now takes the ID as parameter, no longer the token! I'm not sure if any extension might access a token like this. Same goes forfindOrFail().lifetimeparameter was completely removed fromPOST /api/token. I'm hoping we can introduce thetypeparameter to that REST endpoint in a future update, once we decide on whichtypesshould be accessible to users and which should be only created by the system/extensions.whereValidquery scope to only get valid tokens along with shortcutfindValid($token). This is necessary to ensure security even on low activity forums where the garbage collector might not run very frequently or if a bug causes the garbage collector to not trigger.developer. But they could also be named something likeinfinite?Breaking change guide
This is a summary of what we will need to include in the update guide
Most of the deprecated/removed code will trigger E_USER_DEPRECATED PHP errors in Flarum beta 16. Make sure to enable those PHP errors in your PHP config while testing your code with Flarum beta 16.
Extension API changes
The signature to various method related to authentication have been changed to take
$tokenas parameter instead of$userId. Other changes are the result of the move from$lifetimeto$typeFlarum\Http\AccessToken::generate($userId)no longer accepts$lifetimeas a second parameter. Parameter has been kept for backward compatibility but has no effect. It will be removed in beta 17.Flarum\Http\RememberAccessToken::generate($userId)should be used to create remember access tokens.Flarum\Http\DeveloperAccessToken::generate($userId)should be used to create developer access tokens (don't expire).Flarum\Http\SessionAccessToken::generate()can be used as an alias toFlarum\Http\AccessToken::generate(). We might deprecateAccessToken::generate()in the future.Flarum\Http\Rememberer::remember(ResponseInterface $response, AccessToken $token): passing anAccessTokenhas been deprecated. Pass an instance ofRememberAccessTokeninstead. As a temporary compatibility layer, passing any other type of token will convert it into a remember token. In beta 17 the method signature will change to accept onlyRememberAccessToken.Flarum\Http\Rememberer::rememberUser()has been deprecated. Instead you should create/retrieve a token manually withRememberAccessToken::generate()and pass it toRememberer::remember()Flarum\Http\SessionAuthenticator::logIn(Session $session, $userId)second parameter has been deprecated and is replaced with$token. Backward compatibility is kept. In beta 17, the second parameter method signature will change toAccessToken $token.AccessToken::generate()now saves the model to the database before returning it.AccessToken::find($id)or::findOrFail($id)can no longer be used to find a token, because the primary key was changed fromtokentoid. Instead you can useAccessToken::findValid($tokenString)AccessToken::findValid($tokenString): AccessTokenorAccessToken::whereValid(): Illuminate\Database\Eloquent\Builderto find a token. This will automatically scope the request to only return valid tokens. On forums with low activity this increases the security since the automatic deletion of outdated tokens only happens every 50 requests on average.Symfony session changes
If you are directly accessing or manipulating the Symfony session object, the following breaking changes have been made without any backward compatibility:
user_idattribute is no longer used. It's no longer set by Flarum, and setting it won't have any effect.access_tokenhas been added. It's a string that maps to thetokencolumn of theaccess_tokensdatabase table.To retrieve the current user from inside a Flarum extension, the ideal solution which was already present in Flarum is to use
$request->getAttribute('actor')which returns aUserinstance (which might beGuest)To retrieve the token instance from Flarum, you can use
Flarum\Http\AccessToken::findValid($tokenString)To retrieve the user data from a non-Flarum application, you'll need to make an additional database request to retrieve the token. The user ID is present as
user_idon theaccess_tokenstable.Token creation changes
The
lifetimeproperty of access tokens has been removed. Tokens are now eithersessiontokens with 1h lifetime after last activity, orsession_remembertokens with 5 years lifetime after last activity.The
rememberparameter that was previously available on thePOST /loginendpoint has been made available onPOST /api/token. It doesn't return the remember cookie itself, but the token returned can be used as a remember cookie.The
lifetimeparameter ofPOST /api/tokenhas been deprecated and will be removed in Flarum beta 17. Partial backward compatibility has been provided where alifetimevalue longer than 3600 seconds is interpreted likeremember=1. Values lower than 3600 seconds result in a normal non-remember token.New
developertokens that don't expire have been introduced, however they cannot be currently created through the REST API. Developers can create developer tokens from an extension usingFlarum\Http\DeveloperAccessToken::generate($userId).If you manually created tokens in the database from outside Flarum, the
typecolumn is now required and must containsession,session_rememberordeveloper. Tokens of unrecognized type cannot be used to authenticate, but won't be deleted by the garbage collector either. In a future version extensions will be able to register custom access token types.Token usage changes
A security issue in Flarum (#2075) previously caused all tokens to never expire. This had limited security impact due to tokens being long unique characters. However custom integrations that saved a token in an external database for later use might find the tokens no longer working if they were not used recently.
If you use short-lived access tokens for any purpose, take note of the expiration time of 1h. The expiration is based on the time of last usage, so it will remain valid as long as it continues to be used.
Due to the large amount of expired tokens accumulated in the database and the fact most tokens weren't ever used more than once during the login process, we have made the choice to delete all access tokens a lifetime of 3600 seconds as part of the migration, All remaining tokens have been converted to
session_remembertokens.Remember cookie
The remember cookie still works like before, but a few changes have been made that could break unusual implementations.
Now only access tokens created with
rememberoption can be used as remember cookie. Any other type of token will be ignored. This means if you create a token withPOST /api/tokenand then place it in the cookie manually, make sure you setremember=1when creating the token.Web session expiration
In previous versions of Flarum, a session could be kept alive forever until the Symfony session files were deleted from disk.
Now sessions are linked to access tokens. A token being deleted or expiring will automatically end the linked web session.
A token linked to a web session will now be automatically deleted from the database when the user clicks logout. This prevents any stolen token from being re-used, but it could break custom integration that previously used a single access token in both a web session and something else.
Confirmed
composer test).Required changes: