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

Auth method token_type possibleValues fix #19290

Merged
merged 8 commits into from
Feb 23, 2023

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Feb 22, 2023

When configuring an Auth mount the UI was displaying the following three types: default, batch and service. However, the API lists the following four possible types: default-service, default-batch, batch and service. This caused the following problems:

  1. If the user didn't touch the Token Type dropdown we would send an empty string, which the api defaults to "default-service." However, if the user toggled this dropdown and then reselected default, and saved that value, the API returned with a not valid type error because it doesn't accept type default.

  2. The UI displayed the wrong values. I suspect default in the UI was supposed to generalize default-batch and default-service, however, they mean different things and confused the user.

I have fixed these errors by setting noDefault=true on the dropdown and changing the possibleValues to match the API. Ideally the API's default value would be default-service because this is the value all current auth methods return when you send token_type=''. I went back and forth on whether I should make the default value "default-service" in the UI. However, I settled on following what the API behavior is just in case for some reason in the future another Auth method does not return default-service as the token_type when passing an empty string.

With the fix:
image

When they go to tune their mount it will show default-service because even if they didn't send a token_type the API returns default-service
image

I also went ahead and fixed an active class error on the section tabs in this area by removing a surrounding li element.
Before:
image

After:
image

Notes:

  • the API doesn't list TokenType as a mount config option (it only shows that it's available for tuning). I'll make a ticket for this.
  • The UI has fallen behind on some properties like allowed_response_headers and plugin_version that the API lists but we don't have in the UI (we show allowed_response_headers on secret engine mounts but not on auth methods even though it's in the mount-config model?). I'll follow up with this as well, but thought I'd add a note in case anyone has some background here.

defaultFormValue: 'default',
'The type of token that should be generated via this role. For `default-service` and `default-batch` service and batch tokens will be issued respectively, unless the auth method explicitly requests a different type.',
possibleValues: ['default-service', 'default-batch', 'batch', 'service'],
noDefault: true,
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ivana helped me with the new tooltip language.

@Monkeychip Monkeychip added ui bug Used to indicate a potential bug backport/1.11.x labels Feb 22, 2023
@Monkeychip Monkeychip added this to the 1.13.1 milestone Feb 22, 2023
@Monkeychip Monkeychip marked this pull request as ready for review February 22, 2023 16:48
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

I think for accessibility reasons we should look at fixing the styling rather than removing the li tag from those lists.

Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Good catch on this token_type issue! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants