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

fix(auth): use password type for API key input #9858

Closed
wants to merge 2 commits into from

Conversation

nejch
Copy link

@nejch nejch commented Apr 22, 2024

Description

This uses the password type for the input filed for api_key_value to hide the value and not expose it via autocomplete. This makes it consistent with the client secret input and basic auth password fields that already do this. See screenshots below

I also added a small docs change in a separate commit, let me know if that needs a separate PR.

Motivation and Context

With a plain text type the API key would be shown during entry, which can be an issue when sharing screens or demoing API specs using swagger UI.
It was also remembered across sessions by browsers so it could be exposed via autocomplete.

With this, autocomplete still works with password managers but is not automatically exposed. To my knowledge this shouldn't be a breaking change, WDYT?

I could not find any open issues related to this but let me know if I missed any!

⚒️ with ❤️ by Siemens

How Has This Been Tested?

See screenshots below, following the setting-up.md guide and running the petstore locally.

Screenshots (if appropriate):

before after
image image

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

Comment on lines +13 to +17
1. `cd swagger-ui`
1. `npm install`
1. `npm run dev`
1. Wait a bit
1. Open http://localhost:3200/
Copy link
Author

Choose a reason for hiding this comment

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

I changed this to all 1. so future diffs easier, Markdown should already render this correctly, see https://github.com/swagger-api/swagger-ui/blob/cec33bbe2974f336af968a7cef96fd078a06458f/docs/development/setting-up.md. But I can revert to numbers if needed.

@nejch nejch marked this pull request as ready for review April 22, 2024 10:00
@nejch nejch force-pushed the fix/api-key-auth-password branch from cec33bb to 860c722 Compare May 2, 2024 11:54
@glowcloud
Copy link
Contributor

Hi @nejch,

Thanks for the PR! After consideration, we’ve decided not to merge it, as we think this input should remain visible to users while typing. To address the autocompletion problem, our proposed solution is to simply disable it for token-like secrets. I’ll create an issue for it.

@nejch
Copy link
Author

nejch commented May 7, 2024

Hi @nejch,

Thanks for the PR! After consideration, we’ve decided not to merge it, as we think this input should remain visible to users while typing. To address the autocompletion problem, our proposed solution is to simply disable it for token-like secrets. I’ll create an issue for it.

@glowcloud thanks for your answer! I personally think this makes the behavior inconsistent with the oauth2 authorize flow, since the client secret field for the oauth2 does in fact hide it (which makes sense for inputs with generated secrets IMO). API keys and tokens are almost always copy/pasted rather than typed in manually, unlike (some) passwords. But I may be missing some context here, of course.

.

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