Ensure that the user_credentials are available, and allow
for the metadata table to hold a 512 character long access token.
Depends on D8399
Details
Details
- Reviewers
ashoat kamil michal - Commits
- rCOMM06a3b6b3ff76: [Keyserver] Apply DB Migrations
yarn dev
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Is there anybody else you can ask to help with review? Best to avoid having me as a solo reviewer. I've been getting pulled into a ton of diff reviews lately (more than before) and my time for them is very limited...
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 471 ↗ | (On Diff #28316) | Many issues here:
|
| 495 ↗ | (On Diff #28316) | In our JS codebase, we aim to make awaits really clear, so we have a convention against doing things like this. Instead, await should only ever appear as the first keyword on the right side of an assignment, or otherwise as the first keyword in a statement. Here's a way to rewrite this |
| 498 ↗ | (On Diff #28316) | Are you sure \n works inside single-quote string literals in JS? |
| 504 ↗ | (On Diff #28316) | |
Comment Actions
Is there anybody else you can ask to help with review? Best to avoid having me as a solo reviewer. I've been getting pulled into a ton of diff reviews lately (more than before) and my time for them is very limited..
kamil and michal have both stepped up for JS review. I'll add you as a blocking reviewer after they had a pass
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 471 ↗ | (On Diff #28316) | I'm using the async fs.promise library for the file I/O. Unless you would like me to use something else. |
| 495 ↗ | (On Diff #28316) | I would also like to do something like assert(contents.hasOwnedKey("username"), true. To verify the fields, I know it's a bit overkill, but it is kind of awkward if you have something like "user", "password". The check will succeed, but the logic later on will fail. |
| 498 ↗ | (On Diff #28316) | It does, current output: [NODEM] Please add the following to keyserver/secrets/user_credentials.json:
[NODEM] {
[NODEM] . "username": <user>,
[NODEM] . "password": <pass>,
[NODEM] }
[NODEM]
[NODEM] (node:24611) migration 39 failed.
[NODEM] ENOENT: no such file or directory, open 'secrets/user_credentials.json'
[NODEM] [nodemon] process failed, unhandled exit code (2)
[NODEM] [nodemon]
[NODEM] [nodemon] Either the command has a syntax error,
[NODEM] [nodemon] or it is exiting with reserved code 2.
[NODEM] [nodemon]
[NODEM] [nodemon] To keep nodemon running even after a code 2,
[NODEM] [nodemon] add this to the end of your command: || exit 1
[NODEM] [nodemon]
[NODEM] [nodemon] Read more here: https://git.io/fNOAG
[NODEM] [nodemon]
[NODEM] [nodemon] nodemon will stop now so that you can fix the command.
[NODEM] [nodemon]
[NODEM] [nodemon] Error
[NODEM] at Bus.<anonymous> (/Users/jon/comm/comm/node_modules/nodemon/lib/nodemon.js:156:25)
[NODEM] at Bus.emit (node:events:525:35)
[NODEM] at ChildProcess.<anonymous> (/Users/jon/comm/comm/node_modules/nodemon/lib/monitor/run.js:187:11)
[NODEM] at ChildProcess.emit (node:events:513:28)
[NODEM] at Process.ChildProcess._handle.onexit (node:internal/child_process:293:12) |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 471–473 | See point 3 in my last comment. This is still a useless alias | |
| 477–485 | This is a nit, but we update the type of this collection to return Promise<mixed> instead of Promise<void>, then you could skip async/await and just return the Promise you get from dbQuery directly | |
| 492 | Will this fileHandle automatically clean itself up when it goes out-of-scope, or do we have to call .close() manually? | |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 472–480 ↗ | (On Diff #28462) | I'm not sure how to reducing the usage of async () =>, my flow-fu isn't that strong: Cannot call Map with array literal bound to iterable because a call signature declaring the expected parameter / return type is missing in Promise [1] but exists in function type [2] in index 1 of type argument Yield [3] of the return value of property @@iterator. [prop-missing] |
| 492 | Added a finally block. Just in case. | |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 472–480 ↗ | (On Diff #28462) | Not sure what you're doing differently, but this patch worked for me: diff --git a/keyserver/src/database/migration-config.js b/keyserver/src/database/migration-config.js index 7bed3e454..49dee22ee 100644 --- a/keyserver/src/database/migration-config.js +++ b/keyserver/src/database/migration-config.js @@ -469,15 +469,13 @@ const migrations: $ReadOnlyMap<number, () => Promise<mixed>> = new Map([ [39, ensureUserCredentials], [ 40, - async () => { - // Tokens from identity service are 512 characters long - await dbQuery( - SQL` - ALTER TABLE metadata - MODIFY COLUMN data VARCHAR(1023); - `, - ); - }, + // Tokens from identity service are 512 characters long + () => dbQuery( + SQL` + ALTER TABLE metadata + MODIFY COLUMN data VARCHAR(1023); + `, + ), ], ]); const newDatabaseVersion: number = Math.max(...migrations.keys()); |
Comment Actions
In addition to the inline comments, three notes:
- It's important to keep in mind that new DB setups don't run the migrations. As such, you'll need to update keyserver/src/database/setup-db.js to reflect the changes in the metadata table.
- I think we'll need this check for new setups as well. Can you factor ensureUserCredentials out into a separate file, and call it from setup-db.js to make sure that when somebody sets up a keyserver for the first time, we enforce the presence of user_credentials.json?
- Before landing this, please message the team to explain this new requirement!
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 488–490 ↗ | (On Diff #28462) | This doesn't take into account the Docker setup, which doesn't actually have any files. Instead, we should use getCommConfig to check if the config is present. Sorry I didn't realize this earlier! |
| 493–497 ↗ | (On Diff #28462) | Can we update this error to say something like: User credentials for the keyserver owner must be specified. They can be
specified either by setting the
`COMM_JSONCONFIG_secrets_user_credentials` environmental variable, or by
setting a file at keyserver/secrets/user_credentials.json. The contents
should be a JSON blob that looks like this:
{
"username": <user>,
"password": <pass>
}Please note I stripped the trailing comma at the end, as that technically isn't valid JSON. |
Comment Actions
I think we'll need this check for new setups as well.
Added ensureUserCredentials to setup-db.js, and edited metadata.data to the new VARCHAR length.
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 472–480 ↗ | (On Diff #28462) | was trying without the () =>, which made it just a Promise. Forgot the signature wanted functions, not promises. Thanks |
| 488–490 ↗ | (On Diff #28462) | yea, that's a good point. Added the UserCredentials type just above this part for now, will later refactor the reference in a later diff once it exists. Want to avoid reordering the commits, as it will likely be a lot of busy refactoring work. |
| 493–497 ↗ | (On Diff #28462) | Yea, done. Also got trailing comma. |
Comment Actions
Looks good, just some nits!
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 476–479 ↗ | (On Diff #28592) | Nits:
|
| keyserver/src/user/checks.js | ||
| 13 ↗ | (On Diff #28592) | |
| 27 ↗ | (On Diff #28592) | We typically use underscores and lowercase words to make the string more of an "error code" type thing |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 479 ↗ | (On Diff #28673) | Can we indent this two spaces in, so that it mirrors the indentation of line 476, which is where the template string literal is declared? |
Comment Actions
Fix whitespace
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 479 ↗ | (On Diff #28673) | man, prettier does don't care for this SQL syntax |