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?
| "dependencies": { | ||
| "@tanstack/react-query": "^5.64.1", | ||
| "@usace-watermanagement/groundwork-water": "^3.3.1", | ||
| "@usace-watermanagement/groundwork-water": "^3.9.0", |
There was a problem hiding this comment.
older GWW only supported direct grant
| proxy: { | ||
| "^/cwms-data/timeseries/.*": { | ||
| target: env.CDA_API_ROOT, | ||
| "/cwms-data/timeseries": { |
There was a problem hiding this comment.
I debated doing one single wildcard for the localhost proxy here
There was a problem hiding this comment.
May as well so everything can be tested correctly locally by default.
| "https://localhost:5010/*", | ||
| "http://localhost:*" | ||
| "http://localhost:*", | ||
| "http://127.0.0.1:*" |
There was a problem hiding this comment.
Sometimes it'll launch in loopback, othertimes it's localhost. Figured we'd have both.
There was a problem hiding this comment.
Yep, 127.0.0.1 is the ostensibly "correct" way, but in local development mode having both will make life easier.
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?
| proxy: { | ||
| "^/cwms-data/timeseries/.*": { | ||
| target: env.CDA_API_ROOT, | ||
| "/cwms-data/timeseries": { |
There was a problem hiding this comment.
May as well so everything can be tested correctly locally by default.
| "https://localhost:5010/*", | ||
| "http://localhost:*" | ||
| "http://localhost:*", | ||
| "http://127.0.0.1:*" |
There was a problem hiding this comment.
Yep, 127.0.0.1 is the ostensibly "correct" way, but in local development mode having both will make life easier.
| 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.
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)