Refactor identity/mfa/method/* endpoints to fix bad OpenAPI

[maxb]

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 #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.

[maxb]

This PR is currently a draft as I want to:

• Try out the affected APIs to verify they all still behave as they should -- DONE - validated create/update/list/delete behaviour
• Although not strictly necessary - behaviour should not have changed - review the website documentation, where the same high level concept of not trying to conflate create and update, may inspire improvements to make it easier to read - DONE - documentation updated added to PR

before I submit it for review.

[maxb]

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.

[averche]

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?

[maxb]

• Is this change backwards compatible?

Yes, for all valid invocations. Some invalid invocations now return different errors, more consistent with typical Vault practices

None of these endpoints:

vault read identity/mfa/method
vault read identity/mfa/method/totp   # or duo, okta, pingid
vault delete identity/mfa/method/totp # or duo, okta, pingid

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 ("missing method ID").

Following these changes, they return a more correct HTTP 404 ("unsupported path") for the first one, and HTTP 405 ("unsupported operation") for the rest.

• Are there existing tests to create/update each corresponding method?

This functionality is exercised in:

https://github.com/hashicorp/vault/blob/main/vault/external_tests/mfa/login_mfa_test.go
https://github.com/hashicorp/vault/blob/main/vault/external_tests/identity/login_mfa_totp_test.go

[averche]

This change might warrant a changelog entry. Otherwise, LGTM. Thank you once again for contributing!

[dhuckins]

lgtm!
+10000 for removing duplicated code!

[averche]

A few more minor doc nits :)

[maxb]

OK, so:

• I'll have a think how to adequately summarize this change for the changelog

• @dhuckins has an idea in allow case insensitive uuids in mfa requests #21231 it might be a good idea to discuss and reach a conclusion on, as otherwise we'll have conflicting changes - also, it raises an interesting (to me, anyway) edge case about API design:

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 unsupported path error from Vault. Let me provide an example:

vault path-help identity/group/id/blahblah   # <-- returns help for identity/group/id/{id} endpoint
vault path-help identity/group/id/blah^blah  # <-- 404 unsupported path

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 identity/mfa/method/ APIs are some of very few in Vault which use an even more restrictive custom complex regex, and I'd like to submit an idea for your consideration, about loosening the regex in just those APIs:

What if we got rid of the custom UUID regex and used framework.GenericNameRegex to capture the method_id parameter?

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 identity/mfa/method/ APIs with many of the other Vault APIs. Certain relatively easy mistakes, such as accidentally clipping a character off a UUID, would result in a better error message, identifying an invalid method ID, rather than an unsupported path error.

Also, this would make vault path-help identity/mfa/method/blah return the help message, whereas currently, you'd have to use an actual example UUID in the vault path-help command to access that help.

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:

   U    /identity/mfa/method/totp/admin-destroy
   U    /identity/mfa/method/totp/admin-generate
   U    /identity/mfa/method/totp/generate
R  UD   /identity/mfa/method/totp/{method_id}

In order for that to not be a problem, I'd re-order the definitions of the framework.Path elements so the more specific matches would be tested first.

[hghaf099]

Looks good to me!

[averche]

What if we got rid of the custom UUID regex and used framework.GenericNameRegex to capture the method_id parameter?

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 identity/mfa/method/ APIs with many of the other Vault APIs. Certain relatively easy mistakes, such as accidentally clipping a character off a UUID, would result in a better error message, identifying an invalid method ID, rather than an unsupported path error.

Also, this would make vault path-help identity/mfa/method/blah return the help message, whereas currently, you'd have to use an actual example UUID in the vault path-help command to access that help.

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).

[averche]

I'll have a think how to adequately summarize this change for the changelog

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 :)

[raskchanky]

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.

[schavis]

Suggested change

	## Create Duo MFA Method
	## Create Duo MFA method

Style guide issue: use sentence case for headings

[maxb]

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.

[maxb]

@schavis I disagree with the changes you have requested, because they break with existing established convention within the website/content/api-docs/ directory, across many more files than I am modifying.

Please see this evidence demonstrating that:

$ grep -Frx '### Sample Request' | wc -l
819
$ grep -Frx '### Sample request' | wc -l
0

$ grep -Frx '### Sample Payload' | wc -l
317
$ grep -Frx '### Sample payload' | wc -l
0

$ grep -Frx '### Sample Response' | wc -l
442
$ grep -Frx '### Sample response' | wc -l
0

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.

[maxb]

What if we got rid of the custom UUID regex and used framework.GenericNameRegex to capture the method_id parameter?

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.

[schavis]

Furthermore, it would not make sense to migrate only the headings that I have touched

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 :)

[maxb]

I've added the changelog, and consider this now ready to merge.

I'm seeing some positive sentiment for this, and no negative sentiment, so I'll go ahead with making that change

Once I started work on the change to the method_id parameter regex pattern, I discovered it would be more work than I thought, as I wasn't completely confident that all of the functions receiving this parameter had sufficient preexisting validation that they were being passed a valid method ID. Confirming that, and implementing extra checks if needed, would have increased the complexity of a PR which is already moving a fair bit of code around, so I decided that would be too much scope creep for this PR.

If there is interest from HashiCorp, I'd be willing to take that on afterwards in a separate PR.

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.

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.