Make oauth2 client secret column larger #38770
Conversation
julien-nc
requested review from
rullzer,
nickvergessen,
marcelklehr,
ChristophWurst and
kesselb
| $client->setRedirectUri('https://example.com/'); | ||
| $client->setSecret('b81dc8e2dc178817bf28ca7b37265aa96559ca02e6dcdeb74b42221d096ed5ef63681e836ae0ba1077b5fb5e6c2fa7748c78463f66fe0110c8dcb8dd7eb0305b16d0cd993e2ae275879994a2abf88c68|e466d9befa6b0102341458e45ecd551a|013af9e277374483123437f180a3b0371a411ad4f34c451547909769181a7d7cc191f0f5c2de78376d124dd7751b8c9660aabdd913f5e071fc6b819ba2e3d919|3'); | ||
| $this->clientMapper->insert($client); | ||
| } |
There was a problem hiding this comment.
forgot a addToAssertCount(1) or assertTrue(true). I think phpunit will warn that the test doesn't assert anything
|
nit: reorder your commits and push the test only. we should see failing CI. Then you push the fix and we should see green ci. |
| @@ -49,7 +49,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt | |||
| $table = $schema->getTable('oauth2_clients'); | |||
| if ($table->hasColumn('secret')) { | |||
| $column = $table->getColumn('secret'); | |||
| $column->setLength(256); | |||
| $column->setLength(512); | |||
There was a problem hiding this comment.
Changing migrations later on feels iffy
There was a problem hiding this comment.
@rullzer Not sure what to do here.
If we keep Version011601Date20230522143227 with 256 and add a step that just changes the column length we don't prevent problems when the existing secrets will be encrypted and the update will fail because we try to store values that are too long.
We could:
- change the value in
Version011601Date20230522143227so that people who never ran it have the correct column length before encrypting the existing values - add a step that changes the column length to 512 again for people who already ran the previous step successfully
What do you think?
There was a problem hiding this comment.
Yeah I see the trouble
But you need indeed another one for people that did already run the migration
julien-nc
force-pushed
the
fix/38398/oauth-encrypted-secret-length
branch
3 times, most recently
from
0179d34 to
372d271
Compare
Test only: running. |
|
The new test failed before changing the column size: |
julien-nc
force-pushed
the
fix/38398/oauth-encrypted-secret-length
branch
2 times, most recently
from
864a35e to
ed56f2f
Compare
|
And tests are passing now. @rullzer So the length has been changed in the previous migration and a new step that changes the length has also been added. This way we make sure:
|
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
…evious step has run when it was setting it to 256 Signed-off-by: Julien Veyssier <[email protected]>
julien-nc
force-pushed
the
fix/38398/oauth-encrypted-secret-length
branch
from
ed56f2f to
578bf8c
Compare
refs #38398 (comment)
This also adds a simple test inserting a client with a long secret.
@kesselb Any idea how much chars we can expect at most by giving a 64 chars to https://github.com/nextcloud/server/blob/master/lib/private/Security/Crypto.php#L93 ?