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

Update for optional mfa #47

Merged
merged 5 commits into from
Nov 6, 2023
Merged

Update for optional mfa #47

merged 5 commits into from
Nov 6, 2023

Conversation

rubicola
Copy link
Contributor

@rubicola rubicola commented Nov 3, 2023

🔍 What should we check?

  • Login with and without MFA on works as expected

🍒 What have you changed?

  • Allow user to select MFA type if enrolled in multiple MFA options (noting that Authenticator App will always come in first, and the hasAuthy flag will indicate if also enrolled in SMS)
  • If the user then selects 'SMS/AUTHY', then make another request to POST login to trigger the Authy prompt.
  • Added a logout command in case the token saved to the raisely json expires - this clears it.

Which issue does this solve?
The Raisely API is changing to support multiple MFA options.

The CLI will not work once users enrol in another MFA option.

This PR makes sure to support the existing implementation and also when the Raisely API changes for the login flow which offers these multiple options.

To test, you can run this branch locally and test the package:

  • check out this branch
  • Temporarily hardcode the fetch url here to the Raisely development api
  • then run npm install <path to cli> -g
  • run raisely login (and then raisely logout if your token expires at any point)
  • test scenarios on Raisely develop:
    -- your test user has an authy enrolment (you should get authy prompt)
Screenshot 2023-11-03 at 11 58 54 am
  • test different scenarios on Raisely if you checkout this branch:
    -- your test user has an authenticator app enrolment (to set up you can see test instructions on this PR)
    -- your test user has both an authenticator app enrolment and authy enrolment
    ---- selecting authenticator app and then using otp should work
    ---- selecting authy should send prompt and then using otp should work
Screenshot 2023-11-03 at 12 02 41 pm

Screenshot 2023-11-03 at 12 02 57 pm

@rubicola rubicola requested a review from a team November 3, 2023 01:12
Copy link
Contributor

@chrisjensen chrisjensen left a comment

Choose a reason for hiding this comment

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

Haven't tested, but it looks good to me

const authType = subcodeArray[1];

return {
mfaType: authType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the array order guaranteed?
There's no risk here that we return
{ mfaType: 'AUTHY', hasAuthy: false } when they have both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's guaranteed. The API will return either:

  • 'MFA required:AUTHY'
  • 'MFA required:AUTHENTICATOR_APP'
  • 'MFA required:AUTHENTICATOR_APP:hasAuthy'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, sorry the preference is always 'MFA required:AUTHENTICATOR_APP' first if they have both

Copy link
Contributor Author

@rubicola rubicola Nov 3, 2023

Choose a reason for hiding this comment

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

You can see here that the API now makes the preferred app the Authenticator App, so it will throw that error first if the user has both, unless the user specifies it wants to use Authy (which doesn't happen here in the CLI until after they select Authy).

@rubicola rubicola merged commit 1abae9d into master Nov 6, 2023
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.

2 participants