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(ux): ensure that optional security schema is rendered without padlock. #6839

Merged
merged 3 commits into from
Jan 22, 2021

Conversation

mathis-m
Copy link
Contributor

Description

In case there is a empty security definition included the padlock will not be rendered.

Motivation and Context

Fixes #6767

How Has This Been Tested?

new cypress test

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.

@mathis-m mathis-m changed the title Bug/6767 fix(ux): ensure that optional security schema is rendered without padlock. Jan 16, 2021
@mathis-m mathis-m marked this pull request as draft January 16, 2021 14:49
@mathis-m mathis-m marked this pull request as ready for review January 16, 2021 15:16
@hkosova
Copy link
Contributor

hkosova commented Jan 18, 2021

@mathis-m thanks for the PR!

I would suggest reducing the scope of this PR to just the fix for this specific use case:

security:
  - {}

The other use case where security is optional

security:
  - {}
  - apikeyScheme: []

probably needs UX/design input before going forward. The reason being, if an endpoint supports security even if it's optional, consumers still need to know that this endpoint is secured. Maybe optional security needs a different lock icon or custom wording or something like that.

cc @tim-lai

@mathis-m
Copy link
Contributor Author

mathis-m commented Jan 18, 2021

@hkosova for my understanding if a operation contains anonymous access I would not consider this operation as secure.
E.g.:
Is a login considered secured if the username and password is optional?

But yes from UX perspective it might make sense to mark it optional.
I could imagine adding a gray text before the padlock with text "Optional" for that case.

Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

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

Let's create a new /tests/security/anonymous.js instead of the tests/bugs/6767.js. We can still include the ticket number in the description.

Reasoning: we are going to start deprecating the use of the /tests/bugs directory in favor of a more descriptive test filename/structure, so that related test bugs reside close to the test feature(s) itself.

@mathis-m
Copy link
Contributor Author

mathis-m commented Jan 20, 2021

@tim-lai

Let's create a new /tests/security/anonymous.js instead of the tests/bugs/6767.js. We can still include the ticket number in the description.

done in f14a5a3

Reasoning: we are going to start deprecating the use of the /tests/bugs directory in favor of a more descriptive test filename/structure, so that related test bugs reside close to the test feature(s) itself.

Nice! I think it would be good if this may be stated in CONTRIBUTING.md

@tim-lai
Copy link
Contributor

tim-lai commented Jan 21, 2021

After further consideration, I concur with @hkosova. Let's reduce scope for this PR, to only include a single empty object case. However, we should note in the Cypress test that this is a specific implementation choice, and that it is possible to extend/change in the future.

@mathis-m
Copy link
Contributor Author

After further consideration, I concur with @hkosova. Let's reduce scope for this PR, to only include a single empty object case. However, we should note in the Cypress test that this is a specific implementation choice, and that it is possible to extend/change in the future.

@tim-lai done

@tim-lai tim-lai merged commit eddde95 into swagger-api:master Jan 22, 2021
@tim-lai
Copy link
Contributor

tim-lai commented Jan 22, 2021

@mathis-m Thanks for the updates! PR is now merged!

@hkosova
Copy link
Contributor

hkosova commented Jan 22, 2021

@mathis-m I'm a bit late with the answers 😄 but still:

if a operation contains anonymous access I would not consider this operation as secure.
E.g.:
Is a login considered secured if the username and password is optional?

A login operation is probably not a good example for optional security. The most common use case is an operation that returns different amount of data based on anonymous vs authenticated access. Example: GitHub repo listing API, which returns public repos by default vs both public and private when authenticated. Private data is still secured in this case.

But yes from UX perspective it might make sense to mark it optional.
I could imagine adding a gray text before the padlock with text "Optional" for that case.

Actually UI already marks optional security differently. Such operations are initially displayed with the black locked icon, which serves as an indicator that "credentials are already provided". There are some more details about this design choice in this comment.

image

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.

Operation should be considered anonymous if its security has one empty object
3 participants