Conversation
krowvin
changed the title
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Few comments I had to the reviewers
docs/cli/login.rst
Outdated
|
|
||
| - Override the client ID, OIDC base URL, and scopes: | ||
|
|
||
| ``cwms-cli login --client-id cwms --oidc-base-url https://identity-test.cwbi.us/auth/realms/cwbi/protocol/openid-connect --scope "openid profile"`` |
There was a problem hiding this comment.
this will publish this and make it static, a thing for maintenance?
There was a problem hiding this comment.
maybe just a very generic placeholder URL? The actual URL for for the login should be derived from the CDA target OpenAPI spec.
|
Initial PR of these changes requested to @Enovotny and/or @MikeNeilson Will continue the second half of this PR as it currently only saves the token to disk. Need HydrologicEngineeringCenter/cwms-python#274 to make use of the token that is saved. |
|
If you would like to test this locally: pip install --upgrade git+https://github.com/HydrologicEngineeringCenter/cwms-cli.git@79-standard-login-should-be-supportedthen cwms-cli login |
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Is this just the ability to login right now? I'm not seeing how it integrates with actually calls to CDA - just want to verify if that is intentional right now.
docs/cli/login.rst
Outdated
|
|
||
| - Override the client ID, OIDC base URL, and scopes: | ||
|
|
||
| ``cwms-cli login --client-id cwms --oidc-base-url https://identity-test.cwbi.us/auth/realms/cwbi/protocol/openid-connect --scope "openid profile"`` |
There was a problem hiding this comment.
maybe just a very generic placeholder URL? The actual URL for for the login should be derived from the CDA target OpenAPI spec.
@MikeNeilson that is correct via #155 (comment) I have the issue made here and i'm working on PR that for cwms-python. But if this were to change drastically it could change that PR so asking for initial review. Will request again when I mark it as "ready for review". |
krowvin
changed the title
|
Then we could remove the workaround mentioned above Need to test this more before I mark it ready, submitting here for visibility |
What's off about the URL? at least dev and test should be using the not identityc version now. Can't remember for prod. |
I was having some trouble getting the URL from the swagger spec, but I submitted a PR that enabled me to do so easier |
Alters CDA published OpenID Connect metadata and improves local OIDC
test coverage
Changes:
- remove the invalid HTTP auth scheme("openid") from the oIdConnect
security scheme so Swagger publishes a correct oIdConnectUrl
- add a focused test for openID security scheme
- add debug logging for JWT validation failures to expose the underlying
claim/signature issue during local auth setup
- update local compose seed data so m5hectest has TS ID Creator in SWT,
matching the documented/test write-user expectations
Used this in testing
- HydrologicEngineeringCenter/cwms-cli#155
# Conflicts: # docs/cli/blob.rst # docs/index.rst # tests/commands/test_blob_upload.py
…uld-be-supported # Conflicts: # cwmscli/load/timeseries/timeseries_ids.py
|
I tried this on localhost and was able to use Might take a final peak @MikeNeilson before we merge this in? |
2 participants
Drafting this as I work through it for visibility
Created issue with cwms-python here:
HydrologicEngineeringCenter/cwms-python#274
Once that has been merged in, will update this PR to include those changes and make it so this login method can be used for cli commands.