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

Merged
merged 92 commits into from
Mar 15, 2016
Merged

Auth #2014

merged 92 commits into from
Mar 15, 2016

Conversation

bodnia
Copy link
Contributor

@bodnia bodnia commented Mar 4, 2016

@fehguy authentication feature, could you please review?

Remaining features to implement:

  • 1. move oauth2 authentication into popup view
  • 2. add button for handling required securities
  • 3. add handling security from definitions (this might be in swagger-js @fehguy how do you think?)

@fehguy
Copy link
Contributor

fehguy commented Mar 9, 2016

Nice. I've tested it out, here are some comments:

Header

  • Let's remove the "Click to Authorize" link and replace it with a button, styled like the Explore button, that says Authorize.

Operations

We should standardize on one authorization mechanism on the right side of the operation.

  • Remove the "Click to Authorize" link on the left
  • For all operations which have authorization defined, show the (!) on the right, similar to the OAuth implementation when not logged in. When there are scopes required, mousing over the (!) icon will show them, like with OAuth 2.0.
  • When Logged in, show the blue (i) icon. If there are no scopes required, the mouse over should be disabled--when there are scopes, show them in the pop-up like with OAuth 2.0
  • When clicking the (!) icon, the Authorizations pop-up will appear, like when clicking "Click to Authorize"
  • When clicking the (i) icon (you are logged in), the Authorization pop-up will appear and you can log out
  • Remove the On/Off button that is shown in the OAuth mode
  • When there are no authorizations required, be sure to remove the (!) or (i) icons! Also, since authorizations can be defined at the top-level of the API, we should make sure they are propigated to each operation. Please let me know if you'd like this to happen in swagger-js instead

Pop-up dialog

  • Change the Apply links to be buttons, like in the OAuth 2.0 style
  • Can you make the basic auth username / password labels + text boxes more like a table? Meaning you would align the username and password text, remove the "=", and align the input boxes on the right?
  • Same goes for the API Key entry box, aligned in a table rather than stacked

Misc

  • When running from a web server, I still get random, empty tabs opened. It doesn't seem to happen when running from the file system

Overall looks really great. Just a small amount of fine tuning. Please let me know if you have any questions. We have failing tests, and also need to update the swagger-client library to the most recent.

@bodnia
Copy link
Contributor Author

bodnia commented Mar 13, 2016

@fehguy changed due to comment above, except scopes when oauth2 is defined. If I understand correctly the operation authentication button is for all athorizations from security object. So why are should scopes be shown when hovering on auth button?

@fehguy
Copy link
Contributor

fehguy commented Mar 14, 2016

@bodnia I've re-tested--here's what I see, and also here are answers to your questions.

  1. There is no auth button at the top--the api key section has been removed, but we should add an authorize button (Let's remove the "Click to Authorize" link and replace it with a button, styled like the Explore button, that says Authorize.)
  2. After authorizing, the red "!" does not change to green. I have verified that the authorization succeeded.
    image

For your specific question, the operation authentication should be scoped to the security as defined in the operation definition. So I can create an operation that uses oauth 2.0 and specifies read:pets as required when the securityDefinitions section may also define read:pets and write:pets. In this case, hovering should show read:pets only.

@bodnia
Copy link
Contributor Author

bodnia commented Mar 14, 2016

@fehguy as per first point button authorize is displayed only when security object is on top level as it's something 'required', instead of securityDefinitions. Does this make sense?
Thanks for clarifying second point, I'll change this asap.

@fehguy
Copy link
Contributor

fehguy commented Mar 14, 2016

@bodnia the top link should be based on all the securityDefinitions objects. That will allow the scope to be "all possible" definitions for the API, and swagger-js will apply them when needed.

@bodnia
Copy link
Contributor Author

bodnia commented Mar 14, 2016

@fehguy ok, then I'll revert this change back and show the button for securityDefinitions

@bodnia
Copy link
Contributor Author

bodnia commented Mar 15, 2016

Fixed due to comments, @fehguy could you please review?

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