Add spec-driven Swagger auth controls#1779
Conversation
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Few thoughts of my own with these changes
| background: #fafafa; | ||
| } | ||
|
|
||
| .swagger-auth-bar { |
There was a problem hiding this comment.
I was wondering if this should be tailwind css? Preference?
There was a problem hiding this comment.
What ever is inline with GWW and its usages by USACE?
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Sorry for the delay, thought the review had actually submitted thursday and it had not.
| background: #fafafa; | ||
| } | ||
|
|
||
| .swagger-auth-bar { |
There was a problem hiding this comment.
What ever is inline with GWW and its usages by USACE?
| background: #912018; | ||
| } | ||
|
|
||
| .swagger-ui-host:not(.swagger-login-mode) .swagger-ui .auth-wrapper { |
There was a problem hiding this comment.
I don't think this should be done as the lock icon is informative as to what authentication is required by the endpoint. Swagger also doesn't have the best UI for their auth state (see swagger-api/swagger-ui#4402).
- No lock icon: the endpoint is public. Authentication is not involved with the operation.
- Gray unlocked lock icon: authentication is optional. The endpoint allows anonymous access, but also accepts authenticated requests if credentials are provided.
- Black locked lock icon: authentication is required. The endpoint cannot be called successfully without valid credentials.
The change you are implementing here by not displaying the lock icon suggests that all endpoints are public.
Also, CDA does not correctly define Swagger icon state for black locked lock icons -- but that is a separate issue (that I'm checking on to see if it needs an issue to be created).
There was a problem hiding this comment.
I'm good to remove the drop-down and leave a simple login button that logs you in via AAA or KeyCloak depending on the swagger spec.
I'll do this unless you have thoughts on why we should leave locks out.
My thinking was to have users opt into the swagger locks if they want this route of auth. Intenteded for internal use. But I realize that won't always be the case.
There was a problem hiding this comment.
The locks should stay. Definitely interested in the no/gray/black though. Long term there will be items that won't show up from GETs unless authenticated with the new authorization work.
krowvin
force-pushed
the
swagger-auth-controls
branch
from
acd8ed8 to
ad1efcb
Compare
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
This good, the failing tests are clearly something else, but given you have a PR that should fix at least one of those I would wait to merge until that one is and this one is merged/rebased with it.
3 participants
Summary
Validation
npx.cmd eslint src/pages/swagger-ui/index.jsx --ext js,jsx --report-unused-disable-directives --max-warnings 0npm.cmd run buildScreenshots
With "CWBI Login" selected the locks on the swagger UI are not shown. The hope is to not add login confusion by default. While still allowing the option to authenticate via other means (API key/etc)