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

Updating Okta MFA to use official SDK #15355

Merged
merged 10 commits into from
May 17, 2022
Merged

Updating Okta MFA to use official SDK #15355

merged 10 commits into from
May 17, 2022

Conversation

chrishoffman
Copy link
Contributor

The PR migrates the login MFA functionality to use the official Okta SDK to initiate and verify the MFA enforcement.

@chrishoffman chrishoffman added this to the 1.11.0-rc1 milestone May 10, 2022
vault/login_mfa.go Outdated Show resolved Hide resolved
chrishoffman and others added 2 commits May 10, 2022 16:05
Co-authored-by: swayne275 <swayne@hashicorp.com>
vault/login_mfa.go Show resolved Hide resolved
vault/login_mfa.go Outdated Show resolved Hide resolved
@hghaf099
Copy link
Contributor

This looks great! I just have a couple of questions one of which might be out of the scope of this work.

changelog/15355.txt Show resolved Hide resolved
vault/login_mfa.go Show resolved Hide resolved
vault/login_mfa.go Show resolved Hide resolved
@chrishoffman
Copy link
Contributor Author

I also submitted this issue to the SDK. There is an easy enough workaround but it would be nice to use the transaction id from the verify response natively.

@chrishoffman chrishoffman requested a review from calvn May 16, 2022 15:59
@chrishoffman chrishoffman removed the request for review from raskchanky May 17, 2022 14:01
Copy link
Contributor Author

@chrishoffman chrishoffman left a comment

Choose a reason for hiding this comment

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

@calvn @hghaf099 Can I get another quick review?

vault/login_mfa.go Show resolved Hide resolved
var result pushResult
_, err = client.Do(req, &result)
var result *okta.VerifyUserFactorResponse
_, err = rq.Do(ctx, req, &result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like result is already a pointer now. Should rq.Do be fixed for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrishoffman chrishoffman merged commit 534a8ae into main May 17, 2022
@chrishoffman chrishoffman deleted the mfa-okta-sdk branch May 17, 2022 19:14
Gabrielopesantos pushed a commit to Gabrielopesantos/vault that referenced this pull request Jun 6, 2022
* updating MFA to use official Okta SDK

* add changelog

* Update vault/login_mfa.go

Co-authored-by: swayne275 <swayne@hashicorp.com>

* cleanup query param building

* skip if not user factor

* updating struct tags to be more explicit

* fixing incorrect merge

* worrying that URL construction may change in the future, reimplementing GetFactorTransactionStatus

* adding some safety around url building

Co-authored-by: swayne275 <swayne@hashicorp.com>
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.

None yet

4 participants