Skip to content

Deprecate AddMFADevice and its top-level messages#32516

Merged
codingllama merged 12 commits intomasterfrom
codingllama/deprecate-addmfadevice
Sep 26, 2023
Merged

Deprecate AddMFADevice and its top-level messages#32516
codingllama merged 12 commits intomasterfrom
codingllama/deprecate-addmfadevice

Conversation

@codingllama
Copy link
Copy Markdown
Contributor

Deprecate AddMFADevice and move the majority of tests (apart from exactly one) to rely on AddMFADeviceSync instead.

#20343

@codingllama
Copy link
Copy Markdown
Contributor Author

Branched from #32428.

I'm tempted to do one more test follow up, this time consolidating scenarios and simplifying some of the (multiple) helpers we have. Let's see about that tomorrow. :)

Comment thread lib/auth/helpers_mfa.go

type authClient interface {
AddMFADevice(ctx context.Context) (proto.AuthService_AddMFADeviceClient, error)
type authClientI interface {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the rename?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid name clashes with variables named authClient.

Comment thread lib/auth/auth_test.go

// Create user and set password.
const username = "TestAddMFADevice"
const password = "supersecretpassword!!1!"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too stwong

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has letters, it has numbers, it has special characters. Top notch.

Comment thread lib/web/apiserver_test.go
{
name: "with webauthn",
getAuthPreference: func() types.AuthPreference {
getAuthPreference: func(t *testing.T) types.AuthPreference {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking notes. are we passing in the t, so that when it fails we can get specific details?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The t in the top test function is not the same instance as the t in the t.Run function, so when these fail we get an error message from Go itself (because we are mixing the ts).

This is a common-ish test gotcha, with the other being using t.Fatal (and similar) in goroutines.

I hope this explanation makes sense.

@codingllama
Copy link
Copy Markdown
Contributor Author

Thanks for the quick review!

Base automatically changed from codingllama/register-bug to master September 26, 2023 13:02
@codingllama codingllama added this pull request to the merge queue Sep 26, 2023
Merged via the queue into master with commit 13c2830 Sep 26, 2023
@codingllama codingllama deleted the codingllama/deprecate-addmfadevice branch September 26, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants