Skip to content
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

feat: add mfa indexes #745

Merged
merged 3 commits into from
Oct 11, 2022
Merged

feat: add mfa indexes #745

merged 3 commits into from
Oct 11, 2022

Conversation

J0
Copy link
Contributor

@J0 J0 commented Oct 11, 2022

What kind of change does this PR introduce?

Introduces indexes for mfa to target the following queries:

func FindFactorsByUser(tx *storage.Connection, user *User) ([]*Factor, error) {
 	factors := []*Factor{}
 	if err := tx.Q().Where("user_id = ?", user.ID).Order("created_at asc").All(&factors); err != nil {
 		if errors.Cause(err) == sql.ErrNoRows {
 			return factors, nil
 		}
 		return nil, errors.Wrap(err, "Database error when finding MFA factors associated to user")
 	}
 	return factors, nil
 }

Find Token by Session

func FindTokenBySessionID(tx *storage.Connection, sessionId *uuid.UUID) (*RefreshToken, error) {
 	refreshToken := &RefreshToken{}
 	err := tx.Q().Where("session_id = ?", sessionId).Order("created_at asc").First(refreshToken)
 	if err != nil {
 		if errors.Cause(err) == sql.ErrNoRows {
 			return nil, RefreshTokenNotFoundError{}
 		}
 		return nil, err
 	}
 	return refreshToken, nil
 }

https://github.com/supabase/gotrue/pull/736/files#diff-391a5058b1ace3b7d41e48d0c23859a417795c034274ae3cd5fdf2c4ad6dc7b4R104
Source: https://github.com/supabase/gotrue/pull/736/files#diff-21513bee858a7f7f89598f574f70bcac74a98ddbef23b75201960823301398d0R85

 func FindSessionByUserID(tx *storage.Connection, userId uuid.UUID) (*Session, error) {
 	session := &Session{}
 	if err := tx.Eager().Q().Where("user_id = ?", userId).Order("created_at asc").First(session); err != nil {
 		if errors.Cause(err) == sql.ErrNoRows {
 			return nil, SessionNotFoundError{}
 		}
 		return nil, errors.Wrap(err, "error finding session")
 	}
 	return session, nil
 }

https://github.com/supabase/gotrue/pull/736/files#diff-b4f2ed95a2f612a5c133f50ebc2ee346ccb1cabb36343da567c3a1e336668e94R104

The change will go into the mfa branch and we will then cherry pick the changes for merge into master


Sanity checks:

CleanShot 2022-10-11 at 17 10 44@2x

CleanShot 2022-10-11 at 17 11 38@2x

CleanShot 2022-10-11 at 17 12 40@2x

@J0 J0 requested a review from a team as a code owner October 11, 2022 14:03
models/amr.go Outdated Show resolved Hide resolved
J0 and others added 2 commits October 11, 2022 17:53
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
@J0 J0 merged commit 243364b into mfa Oct 11, 2022
@J0 J0 deleted the j0_add_indexes branch October 11, 2022 15:00
@hf hf mentioned this pull request Oct 11, 2022
J0 added a commit that referenced this pull request Oct 18, 2022
* db: add initial schema

* fix: update migrations

* fix: add additional constraints

* fix: add default

* chore: remove whitespace

* chore: remove email as a type

* fix: prevent updated_at from being nullable

* chore: update comments

* feat:enable and disable mfa

* refactor: remove model files

* fix: pull in master

* refactor: rename backup codes to recovery codes

* fix: update number of user fields

* Update signup_test.go

* test: disable and disable

* feat: add recovery codes api

* feat: add initial modles for factor

* feat: initial enroll endpoint

* feat: initial commit for challenge API

* refactor: add logic for filtering between factorid and simple name

* refactor: split based on factor simple name or id

* refactor: make endpoints idempotent

* chore: undo whitespace change

* chore: remove whitespace

* chore: naming and initial tests

* test: add model test for good measure

* feat: add find factor methods

* fix: change method definitions

* initial commit for verify factor

* test: add tests, remove factor_ prefix

* tests: add http test

* feat:initial verify endpoint

* tests: add more tests

* refactor: remove whitespace changes and minor labels

* fix: update db schema

* refactor: add states for factor_status

* fix: update statuses

* fix: add semicolon

* refactor: change bools to timestamps

* refactor: drop _mfa suffix

* refactor: change recovery code ID type to uuid

* fix: set length of token back to 8

* refactor: revert code consumption time to used_at

* test: add test for FindValidRecoveryCodesByUser

* refactor: convert var names to lowercase

* fix: add error check at end of GenerateRecoveryCodes

* chore: run gofmt -s -w

* refactor: update tests with new naming scheme

* fix: update tests with new names

* chore: renaming

* tests: refactor and add initial enroll factor tests

* fix: add associations from factor to user

* refactor: cleanup

* refactor: cleanup magic strings

* refactor: remove newlines

* chore: introduce new factor type

* chore: backpatch naming

* test: update verify factor test

* refactor: add struct tags

* refactor: add struct tas and change route names

* chore: reintroduce uncommented lines

* refactor: clean up tests

* chore: add misc config vars and errors

* tests: Add additional case for expried challenge + invalid code

* fix: patch test

* chore: add newlines

* chore: remove stray comment

* refactor: delete challenge if expired

* feat: remove unused fields, change recovery_codes from GET to POST

* refactor: remove type field from endpoint

* chore: speed up tests (#564)

On a not-so-good laptop, a full test run went from ~90seconds to less
than 10. Three independent changes were made:

1 - In TruncateAll, use `delete from` instead of `truncate`. While
  truncate is faster for large tables, for small tables, it has
considerably more overhead (my understanding is that delete just flags
the tuple as dead, whereas truncate involves a vacuum-like operation).

2 - In tests, use bcrypt.MinCost instead of bcrypt.DefaultCost

3 - Use a 10 millisecond timeout, instead of 1 second timeout, for
TestHookTimeout

* feat: add admin delete methods

* feat: add unenroll endpoint

* Revert "feat: add admin delete methods"

This reverts commit c65c136.

* fix: change behavior of unenroll

* refactor: strip out /enable and /disable

* Revert "feat: add admin delete methods"

This reverts commit c65c136.

* refactr: move routes to /user

* refactor: modify routes

* chore: remove stray comment

* chore: increase number of requests made so expected error is observed

* Revert "chore: increase number of requests made so expected error is observed"

This reverts commit 5374da8.

* chore: refactor MFA unenroll route

* chore: add updated routes

* feat: admin endpoints

* fix: refactor and update audit log action types

* tests: add additional checks for deletion endpoints

* refactor: remove stray constants

* refactor: remove stray lines and comments

* refactor: remove notion of MFAEnabled

* refactor: remove /recovery_codes endpoint

* feat: add update factor admin endpoint

* feat: add IsMFAEnabled

* refactor: reinstate MFAEnabled

* refactor: make issuer mandatory

* chore: remove created_at field from /challenge

* chore: merge in master

* Add MFA Sessions Fields (#643)

* feat: add session fields

* fix: handle terr

* fix: change column name

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: move constants into models (#646)

* test: check for unverified case

* refactor: move constants into models layer

* chore: test semantic release

* chore: update comment

Co-authored-by: joel@joellee.org <joel@joellee.org>

* feat: Convert QR to SVG (#624)

* initial commit

* chore:add qrcodesize param

* refactor: strip our QRCodeSize, undo session changes

Co-authored-by: joel@joellee.org <joel@joellee.org>

* fix: change method of reading from config

* refactor: remove unused var

* fix: patch gosec errors

* feat: cherry-pick step up changes onto separate branch (#652)

* refactor: cherry pick onto separate branch

* fix: update recovery code files

* feat: cherry-pick partial changes from stepup login branch

* Update models/recovery_code.go

Co-authored-by: joel@joellee.org <joel@joellee.org>

* feat: initial gating of routes requiring 1FA

* chore: add AMR entry

* refactor: update claims

* test: add initial mfa login tests

* refactor: Add sign in method to issueRefreshToken

* fix: distinguish between code logins and recovery code logins

* fix: add concept of first MFA login

* chore: add AMRClaims model content

* fix: patch various errors related to method signature

* fix: types for audit action logging

* fix: patch sql syntax errors

* chore: patch tests for stepup login

* fix: update number of user fields

* refactor: modify access token generation

* fix: step up login

* fix: get tests to pass

* fix: patch gosec errors

* chore: pull in master into mfa (#660)

* refactor: `TruncateAll` for better readability (#650)

* Remove unused GenerateEmailOtp function (#655)

* Remove unused function

* chore: remove helper function

* Update crypto.go

* refactor: configuration with validation (#648)

* feat: use proper ip address (#649)

* refactor: add `GrantParams` for issuing refresh tokens (#659)

* fix: remove instance.go

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
Co-authored-by: joel@joellee.org <joel@joellee.org>

* tests: add notion of first log in

* feat: add session logic

* refactor: remove stepup login

* feat: remove stepup login related details

* refactor: remove unused fields

* chore: remove forgotten file

* chore: merge in master

* fix: remove recovery code logic

* refactor: change types to text, remove dead code

* fix: change verify test expected status code

* refactor: convert challenge_id and factor_id to be uuid's

* refactor: remove GetFactor

* refactor: downcase sql files, shift constants back to respective files

* refactor: remove require1FA

* refactor: modify aal levels to be enum

* refactor: reinstate notion of factor type

* refactor:change types of enroll/unenroll from string to bool

* test: add missing require checks

* fix: add tests for removing factor related sessions on unenroll

* fix: update db sessions in issueRefreshToken as well

* fix: convert issuer to default to siteURL

* refactor: change enroll factor message and remove redundant requireAuthentication check

* refactor: remove success field in verifyFactor Response

* fix: remove success code reutrned and allow unverified factors to be unenrolled

* refactor: add IP address type

* refactor: add IP address type

* fix: return token in mfa verify & update session aal (#682)

* fix: add default value for challenge IP

* fix: patch staticcheck errors

* fix: add partial index to allow multiple empty strings

* refactor: remove FindChallengesByFactorID and add IP address to challenge

* fix: add IP address checks

* fix: minor mfa changes (#692)

* refactor enroll

* fix: omit user, user_id & empty friendly_name from response

* Reinstate auth_tests (#696)

* fix: remove belongs to association on factor

* fix: extract issueRefreshToken logic for MFA

* test:reinstate tests

* fix: revert changes which introduce updateMFASessionAndClaims

* Revert "fix: remove belongs to association on factor"

This reverts commit 10057f7.

Co-authored-by: joel@joellee.org <joel@joellee.org>

* MFA: Remove dead code (#697)

* fix: remove belongs to association on factor

* fix: extract issueRefreshToken logic for MFA

* test:reinstate tests

* fix: revert changes which introduce updateMFASessionAndClaims

* Revert "fix: remove belongs to association on factor"

This reverts commit 10057f7.

* fix: resolve staticcheck errors, remove unused code

Co-authored-by: joel@joellee.org <joel@joellee.org>

* Add Rate limiters for MFA (#698)

* fix: add rate limiters

* chore: add conf

* chore: run gofmt

* fix: add max enrolled factors

* refactor: remove requireAuthentication

* fix: merge challenge and verify rate limits

Co-authored-by: joel@joellee.org <joel@joellee.org>

* Rename err to terr (#723)

* fix: remove belongs to association on factor

* fix: extract issueRefreshToken logic for MFA

* refactor: lowercase totp and rename totp_secret to secret to be more generic

* refactor: remove disabled state

* refactor: rename err -> terr

* Revert "refactor: remove disabled state"

This reverts commit bce773d.

* Revert "refactor: lowercase totp and rename totp_secret to secret to be more generic"

This reverts commit 2b6920f.

* Revert "fix: extract issueRefreshToken logic for MFA"

This reverts commit 2dbd47c.

* Revert "fix: remove belongs to association on factor"

This reverts commit 10057f7.

Co-authored-by: joel@joellee.org <joel@joellee.org>

* fix: merge master into v1 (#726)

* patch: merge master into v1

* chore: run gofmt

* refactor: rename TOTPSecret->Secret

* fix: adjust application code to align with db schema

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: replace mfa/constants.go for enum (#727)

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: add sessions refactors (#728)

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: add mfa_test.go refactors (#729)

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: MFA Refactors (#730)

* inital fixes

* Update api/mfa.go

* fix: rename FactorStates

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: update factors.go and getBodyBytes  (#732)

* refactor: change json.NewDecoder -> getBodyBytes

* refactor: update admin to use getBodyBytes

* fix: update comments

* fix: patch error message

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: refactor models/factor and block admin from modifying factor status (#733)

* refactor: change json.NewDecoder -> getBodyBytes

* refactor: update admin to use getBodyBytes

* fix: update comments

* fix: patch error message

* refactor: remove FindFactorByFriendlyName

* chore: update error messages

* fix: refactor unenroll test

* fix: patch staticcheck

* fix: patch stray staticcheck error

* refactor: add test case for unverified factor

* fix: correct error naming and adjust error messages

Co-authored-by: joel@joellee.org <joel@joellee.org>

* fix: update admin tests to add negatives (#734)

Co-authored-by: joel@joellee.org <joel@joellee.org>

* fix: mfa verify should reuse existing session (#691)

* fix: remove belongs to association on factor

* fix: extract issueRefreshToken logic for MFA

* chore: run gofmt

* fix: downgrade other sessions instead of deleting them

* refactor: convert all aal hardcoded strings to use enum

* fix: update AAL Calcuation to not duplicate claims

* fix: reinstate auth_test

* fix: move downgrade to be a method on factor struct

* refactor: merge factor and AAL update methods

* refactor: change mfa_constants to enum

* refactor: change signInMethod to Enum

* refactor: change hardcoded strings to test constants

* refactor: rename secondary session variables

* test: add check that unenrolling clears factorID from assoc session

* refactor: remove unused structs

* refactor: nit changes (#694)

* refactor: validate totp only if challenge isn't expired

* refactor: better error handling

* refactor: read from DB session instead of JWT token

* refactor: remove outdated test

* test: add tests for calculate AAL and AMR

* fix: add test to ensure claims are not duplicated

* fix: add ordering condition

* refactor: rename signInMethod to authenticationMethod

* fix: change verify bad code return from 401->400

* chore: run gofmt

* fix: re-read association from session

* refactor: change methods using enum types to AuthenticationMethod

* fix: change more functions to take in enum

* test: add integration tests

* test: add test to check AAL maintainance

* fix: add test for refresh token rotation

* chore: fix staticcheck

* refactor: move sessionID calculation out from transaction

* refactor: remove authentication method map

* refactor: remove additional CalculateAALAndAMR

* fix: remove requirement for tokens to be not revoked

* Apply suggestions from code review

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

* fix: gofmt, refactor return type for enrollfactor

* refactor: add comments to clarify tests, remove unused codew

* fix: add index to refresh_token

* refactor: minor renaming and add check on qrcode contents

* fix: replace expires in to default expiry

* fix: update unverified factor test

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

* chore: remove dead code

* refactor: add explicit struct tags(mfa) (#740)

* refactor: explicitly name structs

* refactor: add explicit tags to user and phone

* refactor: add admin related tags

Co-authored-by: joel@joellee.org <joel@joellee.org>

* fix: trigger token swap on MFA verify to ensure token is latest one (#742)

* fix: trigger token swap to ensure token is latest one

* test: add additional test to check 2FA followed by 1FA sign in

* Update api/token.go

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

* Revert "Update api/token.go"

This reverts commit f6aa576.

* refactor: reduce number of params needed

* refactor: move FindSessionByUserID to tests

* Revert "refactor: move FindSessionByUserID to tests"

This reverts commit 3b56457.

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

* chore: add feature flag (#737)

* fix: add initial flags on tests

* fix: add MFA_ENABLED flag to tests

* fix: checks in test admin

* fix: add enabled flag

* fix: properly make use of config

* fix: remove stray env var addition

* feat: initial prefixing

* fix: prefix mfa related tests

* Update models/refresh_token.go

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

* fix: update comparison checks at

* chore: MFA_ENABLED->GOTRUE_MFA_ENABLED

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

* refactor: mfa without user in route, other tiny fixes (#743)

* refactor: add unenroll factor response

* refactor: remove /user/<prefix> route for mfa

* feat: add mfa indexes (#745)

* feat: add mfa indexes

* Update models/amr.go

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

* Update migrations/20221011041400_add_mfa_indexes.up.sql

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

* fix: return mfa enabled in settings (#747)

* refactor: better error messages for mfa (#751)

* refactor: rename errors

* fix: add aal2 check

* refactor: simplify amr claims

* fix: add additional test for unenrolling verified factors

* Apply suggestions from code review

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>

* refactor: remove requirement for code in order to unenroll (#753)

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: pluralize factors endpoint, use lowercase `totp` (#752)

* test: add sanity checks to ensure secret does not leak (#755)

* refactor: remove requirement for code in order to unenroll

* test: add sanity checks for secret leakage

* fix: update status codes for unenroll factor

Co-authored-by: joel@joellee.org <joel@joellee.org>

* refactor: sort amr claims as most-recent first (#756)

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: Karl Seguin <karlseguin@users.noreply.github.com>
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants