Add DB changes #496
Add DB changes #496
Conversation
| @@ -0,0 +1,47 @@ | |||
| -- See: https://stackoverflow.com/questions/7624919/check-if-a-user-defined-type-already-exists-in-postgresql/48382296#48382296 | |||
| DO $$ BEGIN | |||
| CREATE TYPE factor_type AS ENUM('phone', 'webauthn'); | |||
There was a problem hiding this comment.
can we just solve this by adding a namespace and prefix the factor type with mfa- ?
There was a problem hiding this comment.
Hmm do you mean like:
CREATE TYPE mfa.factor_type AS ENUM('mfa-phone', 'mfa-webauthn');?
There was a problem hiding this comment.
yup, or even phone-factor , webauthn-factor whichever you prefer
There was a problem hiding this comment.
Hmm oh how would that help to guard against factor_type already existing though?
It's not that clear but was hoping to guard against the following scenario:
- Migrations are all applied, GoTrue is running
- An error occurs and the
schema_migrationstable is cleared in order to trigger migrations to apply again - A restart occurs and GoTrue tries to reapply migrations, migrations prior to
add_mfa_schemaare ignored since the tables/changes already exist - The
add_mfa_migrationis run again butfactor_typealready exists and thus an error would be thrown if recreation is attempted
Might be missing something but at step 4. seems like we would also run into a type already exists error when factor_type is created under an mfa schema as well
There was a problem hiding this comment.
hmm how would (2) occur, the schema_migrations table wouldn't be dropped by the migration tool unless the user does it explicitly right?
There was a problem hiding this comment.
Yep the main use case I had in mind was a support ticket with migration issues
| CREATE TABLE IF NOT EXISTS auth.mfa_factors( | ||
| id VARCHAR(256) NOT NULL, | ||
| user_id uuid NOT NULL, | ||
| friendly_name VARCHAR(256) NULL, |
There was a problem hiding this comment.
friendly_name is just an alias that can be specified by the developer right?
There was a problem hiding this comment.
hmm seems like there's a unique constraint on (user_id, friendly_name), so i guess friendly_name will be used to identify one of the mfa factors associated to a user?
There was a problem hiding this comment.
Yup friendly name is just an alias specified by developer and each user should only be able to specify one factor with given alias (e.g. user123 shouldn't have two factors named "bob" or similar.
|
|
||
| -- auth.mfa_recovery_codes definition | ||
| CREATE TABLE IF NOT EXISTS auth.mfa_recovery_codes( | ||
| id serial PRIMARY KEY, |
There was a problem hiding this comment.
just wondering, why is this serial while the other tables are using varchar(256) for the id type?
There was a problem hiding this comment.
also, if you wanna go with serial, you should consider using bigint generated always as identity since this would accommodate up to 8bytes (similar to varchar(256) but autoincrementing)
There was a problem hiding this comment.
Yup great question -- the other two are meant to be of the form prefix_<uuid> (e.g. challenge_1231524) since they are often used together in client lib/endpoint calls and it's purely to help the dev distinguish between the various uuids.
For recovery codes the id is not likely to be used elsewhere/exposed to user so there haven't been any measures. Open to changing it to varchar too though
| valid BOOLEAN NOT NULL, | ||
| created_at timestamptz NOT NULL, | ||
| used_at timestamptz NOT NULL, | ||
| CONSTRAINT mfa_recovery_codes_user_id_recovery_code_pkey UNIQUE(user_id, recovery_code), |
There was a problem hiding this comment.
should this be UNIQUE(factor_id, recovery_code) instead? or are we allowing a user to use a recovery code to bypass any of the factors?
There was a problem hiding this comment.
Yup, good question -- recovery codes are per user instead of per factor. This seems to be the case for most other auth providers -- User can use any one of the eight recovery codes to log in.
This is also quite dangerous so we'll have to guard it well.
What kind of change does this PR introduce?
Initial DB schema for MFA. Will add the indexes later in the implementation once I have a clearer idea of what to index
Note that I've changed the base branch to
mfaand will be usingj0_add_mfaas a draft branchMain Changes since last review
factor_statusto differentiate betweendisabledfactors andunverifiedfactors. Factors can only be disabled once they are verified.verificationstatus. Challenges are marked as verified once there is a successful verificationfactor_prefix on the factor table and changedsimple_nametofriendly_name