refactor: MFA Refactors #730
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
J0
commented
Oct 4, 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?