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

chore: Migrate Two FA from packngo to equinix-sdk-go client #353

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented Sep 27, 2023

Issue Task as part of migrating metal-cli from packngo to metal-go client, added the support of Two Fa to use metal-go
Fixes: #333

Discussion:
As of metal-go 0.22.2 there are 2 issues which needs api support

  • Accepting otp code in the input for Enable and Disable 2FA is not supported from metal-go
  • Receiving an otp on two fa registered app is also not supported

@aayushrangwala aayushrangwala temporarily deployed to external September 27, 2023 13:42 — with GitHub Actions Inactive
@aayushrangwala aayushrangwala changed the title Migrate Two FA from packngo to metal-go client Draft: Migrate Two FA from packngo to metal-go client Sep 27, 2023
@aayushrangwala aayushrangwala marked this pull request as draft September 27, 2023 13:51
@aayushrangwala aayushrangwala changed the title Draft: Migrate Two FA from packngo to metal-go client fix: Migrate Two FA from packngo to metal-go client Sep 27, 2023
@cprivitere cprivitere temporarily deployed to external October 9, 2023 19:15 — with GitHub Actions Inactive
@displague displague changed the title fix: Migrate Two FA from packngo to metal-go client chore: Migrate Two FA from packngo to metal-go client Nov 9, 2023
@displague
Copy link
Member

@aayushrangwala please open an issue and create a patches/ PR in Metal Go for the missing TFA parameters (which we can piece together from the packngo implementation). We can rebase this PR against metal go with those changes.

@displague
Copy link
Member

This is one of two final packngo dependent services. (the other being facility listing)

@aayushrangwala
Copy link
Contributor Author

This is one of two final packngo dependent services. (the other being facility listing)

Yes, we do have an issue open with API team. Although seems it has not been prioritised yet

displague added a commit to equinix/equinix-sdk-go that referenced this pull request Jan 22, 2024
…25)

Conversion of [`metal-cli` 2-factor auth
subcommands](equinix/metal-cli#353) from
`packngo` to `equinix-sdk-go` is blocked because the Equinix Metal
OpenAPI spec is missing many details for MFA setup and validation.

This works around the API issue by patching the local SDK spec to fill
in the missing MFA details. These patches were written based on the
[`packngo` MFA
implementation](https://github.com/packethost/packngo/blob/f8b3f36ba929a29d4f1d29480e0f47c7f85790e4/two_factor_auth.go).
@ctreatma
Copy link
Contributor

equinix-sdk-go@v0.32.0 contains a workaround for the missing OTP/MFA spec endpoints that are needed for this PR

@ctreatma ctreatma changed the title chore: Migrate Two FA from packngo to metal-go client chore: Migrate Two FA from packngo to equinix-sdk-go client Jan 24, 2024
cmdFunc func(*testing.T, *cobra.Command)
}{
{
name: "receive sms",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is only testing the receive subcommand because I haven't recreated a successful scenario with the enable and disable commands. For the other 2fa subcommands, the same failure happens in both the packngo and equinix-sdk-go implementations:

HTTP/1.1 424 Failed Dependency
{"errors":["Invalid OTP"]}

Since this is a pre-existing issue and isn't caused or fixed by the SDK migration, I'm advocating to handle it in a separate issue & PR.

@ctreatma ctreatma marked this pull request as ready for review January 24, 2024 18:07
@displague
Copy link
Member

Is #59 also addressed by this PR?

@ctreatma
Copy link
Contributor

Is #59 also addressed by this PR?

This PR does not address that issue but that issue appears to have already been addressed.

@ctreatma ctreatma merged commit 9576704 into equinix:main Jan 24, 2024
7 of 8 checks passed
This pull request was closed.
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.

Adopt metal-go
4 participants