-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Make oauth2 client secret column larger #38770
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
/** | ||
* @copyright Copyright 2023, Julien Veyssier <[email protected]> | ||
* | ||
* @author Julien Veyssier <[email protected]> | ||
* | ||
* @license GNU AGPL version 3 or any later version | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as | ||
* published by the Free Software Foundation, either version 3 of the | ||
* License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
namespace OCA\OAuth2\Migration; | ||
|
||
use Closure; | ||
use OCP\DB\ISchemaWrapper; | ||
use OCP\Migration\IOutput; | ||
use OCP\Migration\SimpleMigrationStep; | ||
|
||
class Version011602Date20230613160650 extends SimpleMigrationStep { | ||
|
||
public function __construct( | ||
) { | ||
} | ||
|
||
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { | ||
/** @var ISchemaWrapper $schema */ | ||
$schema = $schemaClosure(); | ||
|
||
if ($schema->hasTable('oauth2_clients')) { | ||
$table = $schema->getTable('oauth2_clients'); | ||
if ($table->hasColumn('secret')) { | ||
$column = $table->getColumn('secret'); | ||
// we still change the column length in case Version011601Date20230522143227 | ||
// has run before it was changed to set the length to 512 | ||
$column->setLength(512); | ||
return $schema; | ||
} | ||
} | ||
|
||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,4 +84,14 @@ public function testGetByUidNotExisting() { | |
public function testGetClients() { | ||
$this->assertSame('array', gettype($this->clientMapper->getClients())); | ||
} | ||
|
||
public function testInsertLongEncryptedSecret(): void { | ||
$client = new Client(); | ||
$client->setClientIdentifier('MyNewClient'); | ||
$client->setName('Client Name'); | ||
$client->setRedirectUri('https://example.com/'); | ||
$client->setSecret('b81dc8e2dc178817bf28ca7b37265aa96559ca02e6dcdeb74b42221d096ed5ef63681e836ae0ba1077b5fb5e6c2fa7748c78463f66fe0110c8dcb8dd7eb0305b16d0cd993e2ae275879994a2abf88c68|e466d9befa6b0102341458e45ecd551a|013af9e277374483123437f180a3b0371a411ad4f34c451547909769181a7d7cc191f0f5c2de78376d124dd7751b8c9660aabdd913f5e071fc6b819ba2e3d919|3'); | ||
$this->clientMapper->insert($client); | ||
$this->assertTrue(true); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. forgot a addToAssertCount(1) or assertTrue(true). I think phpunit will warn that the test doesn't assert anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $this-> |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing migrations later on feels iffy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
Version011601Date20230522143227
so that people who never ran it have the correct column length before encrypting the existing valuesWhat do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see the trouble
But you need indeed another one for people that did already run the migration