[Browser MFA] Split validation out of Finish function#63978
[Browser MFA] Split validation out of Finish function#63978danielashare wants to merge 5 commits into
Conversation
danielashare
added
the
no-changelog
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
I appreciate the small, focused PR. Thanks!
This doesn't need to depend on danielashare/browser-mfa-proto - if you drop those commits it can land on its own.
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| err := webLogin.Validate(ctx, test.user, test.createResp(), test.exts) |
There was a problem hiding this comment.
Similarly, should we piggy back this onto TestLoginFlow_Finish_errors? That's even simpler with "createResp" being a func.
There was a problem hiding this comment.
As above, combined the two
| // Validate should succeed and not consume the session. | ||
| err = webLogin.Validate(ctx, user, assertionResp, &mfav1.ChallengeExtensions{ |
There was a problem hiding this comment.
Is this test covering a meaningful scenario?
Is a reusable session any different from a "normal" one? Do we expect multiple Validate calls for a reusable session?
There was a problem hiding this comment.
Yeah, it wouldn't be validated twice, I removed this test
danielashare
changed the base branch from
danielashare/browser-mfa-proto
to
master
danielashare
force-pushed
the
danielashare/browser-mfa-webauthn-validation
branch
from
9f7bc17 to
dd9ef28
Compare
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Please take a look at the test failures.
| user string, | ||
| resp *wantypes.CredentialAssertionResponse, | ||
| requiredExtensions *mfav1.ChallengeExtensions, | ||
| validateOnly bool, |
There was a problem hiding this comment.
Just to clarify, I was OK keeping the public variants of Finish and Validate. The refactor was more about having them both delegate to the private finish func.
There was a problem hiding this comment.
That's cleaner, added
| exts: &mfav1.ChallengeExtensions{ | ||
| Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN, | ||
| }, |
There was a problem hiding this comment.
Pull into okExts (or some other name) and reuse throughout?
| exts: &mfav1.ChallengeExtensions{ | |
| Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN, | |
| }, | |
| exts: okExts, |
danielashare
changed the base branch from
master
to
danielashare/browser-mfa-proto
danielashare
requested review from
camscale,
doggydogworld,
espadolini,
fheinecke,
fspmarshall,
hugoShaka,
klizhentas,
probakowski,
r0mant,
rosstimothy,
russjones,
strideynet,
tigrato and
zmb3
as code owners
danielashare
force-pushed
the
danielashare/browser-mfa-webauthn-validation
branch
from
1b45bb0 to
9290713
Compare
danielashare
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
|
@danielashare - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
danielashare
requested review from
codingllama
and removed request for
camscale,
doggydogworld,
fheinecke,
fspmarshall,
hugoShaka,
mjsmithnh,
probakowski,
rosstimothy,
russjones,
strideynet,
tigrato and
zmb3
danielashare
force-pushed
the
danielashare/browser-mfa-webauthn-validation
branch
4 times, most recently
from
6a05b78 to
a1f454e
Compare
|
Apologies for the mass review ping, attempted git surgery did not go well |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Looks great, thanks Dan!
| } | ||
| } | ||
|
|
||
| func (f *fakeIdentity) clone() *fakeIdentity { |
There was a problem hiding this comment.
If this were a clone function in prod code, I'd probably ask for some kind of fuzz test to ensure new fields that are added are covered. As this is test code, that seems unnecessary - but we could add a short note where the struct is defined to remind other engineers in future that they may need to update the clone.
strideynet
left a comment
strideynet
left a comment
There was a problem hiding this comment.
Looks good - thanks for running manual tests 👍
public-teleport-github-review-bot
Bot
removed the request for review
from tele-lion
danielashare
force-pushed
the
danielashare/browser-mfa-webauthn-validation
branch
from
a25407d to
b344b73
Compare
|
Closing due to double validation requirement being dropped in #63873 |
3 participants
This PR extracts the validation logic out of the WebAuthn finish function. This allows the Browser MFA flow to validate a WebAuthn response it gets from the browser before sending it to
tshto be consumed. The RFD for this addition can be found here.Manual tests: