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.

Sign up for GitHub

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

Jump to bottom

Add support for CLI-based MFA recovery #458

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Add support for CLI-based MFA recovery #458

merged 2 commits into from
Jun 13, 2024

Conversation

nmanoogian
Copy link
Member

@nmanoogian nmanoogian commented Jun 5, 2024 •
edited
Loading

This PR adds a CLI command which simply calls POST /v3/me/mfa_recovery.

This depends on https://github.com/DopplerHQ/server/pull/6727

@nmanoogian nmanoogian requested a review from a team as a code owner June 5, 2024 15:36
@apazzolini apazzolini self-requested a review June 5, 2024 16:33
apazzolini
apazzolini previously approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@apazzolini apazzolini left a comment

Choose a reason for hiding this comment

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

Will defer to your judgment on the comment.

pkg/cmd/mfa-recovery.go Outdated
)

var mfaRecoveryCmd = &cobra.Command{
Use: "mfa-recovery",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the command doppler mfa recovery for more symmetry with the other options? (And in case we want to do other things with mfa in the future, e.g. allow the CLI to itself be an mfa option).

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also potentially nest it under doppler login (i.e. doppler login mfa-recovery).

Copy link
Member Author

Choose a reason for hiding this comment

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

I like both of these options! I'll go with doppler mfa recovery as I like the idea of having more MFA commands in the future

@nmanoogian nmanoogian requested a review from apazzolini June 7, 2024 20:16
@nmanoogian nmanoogian force-pushed the nic/mfa-recovery branch 22 times, most recently from bc3f94c to 6622694 Compare June 7, 2024 22:05
@nmanoogian nmanoogian force-pushed the nic/mfa-recovery branch 2 times, most recently from 9302308 to 34c9047 Compare June 7, 2024 22:12
@nmanoogian nmanoogian force-pushed the nic/mfa-recovery branch 11 times, most recently from e98bb90 to bbbba91 Compare June 13, 2024 15:27
@nmanoogian
Copy link
Member Author

We're seeing an issue with our Windows install.sh test in that the response code of the download request is coming back "000" with an error code from curl. It's pretty easy to test with:

curl --tlsv1.2 --proto "=https" -w "%{http_code}" --silent --retry 5 -o "binary" -LN -D - "https://cli.doppler.com/download?os=windows&arch=amd64&format=zip" 2>&1

Thanks to @watsonian for finding this issue: curl/curl#13845 which appears to be the same problem.

I've removed the Windows test as a required check for now and I'll monitor the curl issue for updates.

@nmanoogian nmanoogian merged commit 54d5ad9 into master Jun 13, 2024
27 of 28 checks passed
@nmanoogian nmanoogian deleted the nic/mfa-recovery branch June 13, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piccirello Piccirello Piccirello left review comments

@apazzolini apazzolini apazzolini approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nmanoogian @apazzolini @Piccirello