-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Refactor identity/mfa/method/*
endpoints to fix bad OpenAPI
#20879
Refactor identity/mfa/method/*
endpoints to fix bad OpenAPI
#20879
Conversation
There is a problem with how the `identity/mfa/method/*` endpoints are defined, resulting in incorrect OpenAPI generation. I raised hashicorp/vault-client-go#180 to track a consequence, and opened hashicorp#20873 which explains the problem and adds a log message to detect it. This PR is now the fix. It's actually quite an interesting problem, that has come about through some particular implementation choices, in Vault's first/only case where REST API objects are created by writing to the collection URL, and have their ID allocated by the server, instead of the client. The triggering cause of the malfunction was trying to have a single framework.Path struct instance which optionally includes or excludes the method_id path parameter, and also another framework.Path struct instance handling list operations. The fix is to simplify the path regexes, and have one framework.Path which handles the method_id being present, and one that handles it being absent. The diff is somewhat large, because the affected code had been copy/pasted four times (TOTP, Okta, Duo, PingID) - so I took the opportunity to fix the duplication, creating appropriate helper methods so that the quadruplicated code could be re-unified.
This PR is currently a draft as I want to:
before I submit it for review. |
Type: framework.TypeString, | ||
Description: `The unique name identifier for this MFA method.`, | ||
}, | ||
"max_validation_attempts": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Advice for people reviewing this PR: If you turn on the "Ignore whitespace changes" diff mode, you'll be able to see these long definitions of field schemas are not being changed, only re-indented upon being moved to a different place in the code.
This update refactors how the documentation presents these endpoints to users, both for clarity, and to align with the new structure of the code. From a user perspective, it clears up some unclear presentation of when the `method_id` parameter should and should not be present, adds a missing description of the response to create requests, and changes the `method_id` parameter name to be used consistently (rather than `id` in some cases, unlike the actual code/OpenAPI).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed review, we've been quite busy with the 1.14 release.
Overall this change makes sense to me but I'd like some folks from @hashicorp/vault-ecosystem to take a look as well.
A couple question though:
- Is this change backwards compatible?
- Are there existing tests to create/update each corresponding method?
Yes, for all valid invocations. Some invalid invocations now return different errors, more consistent with typical Vault practices None of these endpoints:
were intended to exist, but due to the looseness of the regex, currently exist in a broken form, always resulting in an HTTP 400 error ( Following these changes, they return a more correct HTTP 404 (
This functionality is exercised in: https://github.com/hashicorp/vault/blob/main/vault/external_tests/mfa/login_mfa_test.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change might warrant a changelog entry. Otherwise, LGTM. Thank you once again for contributing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
+10000 for removing duplicated code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more minor doc nits :)
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
OK, so:
At the OpenAPI level, each path parameter is just a placeholder. It doesn't come with a filtering regex. But Vault isn't like that, so depending on the values you fill in to the parameters, you can call what according to the OpenAPI is a valid endpoint, yet get back an
This demonstrates a reason to be hesitant about putting too much validation into the regexes, rather than just capturing parameters, and doing further validation in code which can produce better errors. I don't propose we change this across every API doing something similar in the entirety of Vault. It would be too potentially disruptive for little benefit. But the What if we got rid of the custom UUID regex and used A UUID meets the definition of a "generic name" as used in many of Vault's other endpoints. This would harmonize the endpoint matching of the Also, this would make Anyway, it's just an idea, let me know whether it appeals to you or not. It seemed worth bringing up since @dhuckins is already thinking about options for change in this parameter. For completeness, I should point out that TOTP method has some extra endpoints which feature a verb in the same place in the path as the method ID goes:
In order for that to not be a problem, I'd re-order the definitions of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
This sounds reasonable to me. Perhaps @raskchanky could add a bit more context on why we may want a more restrictive regex (it was introduced as part of the #14025). |
I'd say a changelog is nice-to-have but optional here since it's mostly a refactor. If you prefer we can merge it without one :) |
The reason for the different UUID was because it was optional. If a UUID was present, it was considered an update request. If it was absent, it was considered a create request. I didn't realize that approach would cause problems for OpenAPI, but if the new approach is backwards compatible with existing behavior and also fixes OpenAPI, then it sounds good to me. |
@@ -5,18 +5,16 @@ description: >- | |||
The '/identity/mfa/method/duo' endpoint focuses on managing Duo MFA behaviors in Vault. | |||
--- | |||
|
|||
## Configure Duo MFA Method | |||
## Create Duo MFA Method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Create Duo MFA Method | |
## Create Duo MFA method |
Style guide issue: use sentence case for headings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I have a common response to all of the 24 similar suggestions in this review, I've put it as a separate top-level comment below, and marked the other 23 suggestions as resolved, to merge the conversation flow into one place.
@schavis I disagree with the changes you have requested, because they break with existing established convention within the Please see this evidence demonstrating that:
Furthermore, it would not make sense to migrate only the headings that I have touched (or are within close proximity to a line I have touched) within each of these files to a different style. They would then be inconsistent with the other headings within the same files. It is possible you may wish to re-style the entire documentation collection, but if so I suggest that would be better done in its own PR. |
I'm seeing some positive sentiment for this, and no negative sentiment, so I'll go ahead with making that change, and adding a changelog entry, and then I think this should be good to merge. |
I strongly disagree. Enforcing style guide requirements on new commits while performing maintenance to update the rest is standard procedure for a doc set as large as this one. I'm not going to block merging, but please be aware that we've already started correcting the heading styles in recent content PRs so consistency within the full set of Vault docs is already out the window :) |
I've added the changelog, and consider this now ready to merge.
Once I started work on the change to the If there is interest from HashiCorp, I'd be willing to take that on afterwards in a separate PR.
Well, that really depends on which standards a particular project chooses to follow. Some other procedures it is not uncommon to find adopted as project standards are "Maintain consistency within a single page/file, even if there is inconsistency across the wider project." and "Keep content changes and non-trivial amounts of style fixes in separate PRs, so reviewers can focus better on what they are reviewing." It may be useful to write down in CONTRIBUTING.md the standards the Vault project wishes to apply regarding documentation style. |
There is a problem with how the
identity/mfa/method/*
endpoints aredefined, resulting in incorrect OpenAPI generation.
I raised hashicorp/vault-client-go#180 to track a consequence, and
opened #20873 which explains the problem and adds a log message to
detect it.
This PR is now the fix.
It's actually quite an interesting problem, that has come about through
some particular implementation choices, in Vault's first/only case where
REST API objects are created by writing to the collection URL, and have
their ID allocated by the server, instead of the client.
The triggering cause of the malfunction was trying to have a single
framework.Path struct instance which optionally includes or excludes the
method_id path parameter, and also another framework.Path struct
instance handling list operations.
The fix is to simplify the path regexes, and have one framework.Path
which handles the method_id being present, and one that handles it being
absent.
The diff is somewhat large, because the affected code had been
copy/pasted four times (TOTP, Okta, Duo, PingID) - so I took the
opportunity to fix the duplication, creating appropriate helper methods
so that the quadruplicated code could be re-unified.