FEAT 222 Add client credential flow #228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 93.94% 94.06% +0.11%
==========================================
Files 8 8
Lines 793 809 +16
==========================================
+ Hits 745 761 +16
Misses 48 48
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Enabling PKCE seems uncontroversial, we support it in MI, not sure if we might need to support APIs without it, but that seems like speculative generality at this point.
More of a question is storing client_id and client_secrets, I wonder if we would need to store them in the cred store and use them from there.
| Resource owner username. Provided by the Identity provider. | ||
| client_secret : :class:`str` | ||
| Resource owner password. Provided by the Identity provider. | ||
| scope : :class:`str`, optional |
There was a problem hiding this comment.
How do we provide multiple scopes here, using a comma separated list?
There was a problem hiding this comment.
Good point. In re-reading the requests-auth docs, the docstring there says "Scope parameter sent to token URL as body. Can also be a list of scopes." - my interpretation of this is that it can be str or list[str]. https://colin-b.github.io/requests_auth/#client-credentials-flow
| If the OIDC features have not been installed. | ||
| """ | ||
|
|
||
| @wraps(func) |
src/ansys/openapi/common/_session.py
Outdated
|
|
||
| def with_oidc( | ||
| @require_oidc | ||
| def with_oidc_client_credentials( |
There was a problem hiding this comment.
I suspect we may want to get these from the credential store as well. but this is fine for now
There was a problem hiding this comment.
I'd say this is similar to the issue with the tokens for the other OIDC flow. We might want to implement that here, or we might want to leave it up to the code that's using this package to decide how they want to handle it.
|
|
||
| def with_oidc( | ||
| @require_oidc | ||
| def with_oidc_client_credentials_flow( |
There was a problem hiding this comment.
I feel like it would make more sense to take the token_url here, and leave the api_url as the actual service_url. We could always fall back to the api_url as the token_url if it isn't specified, but it just seems a bit clumsy right now.
There was a problem hiding this comment.
We should get all the information we need in the initial 401 response from the API url and from the well known endpoint at the identity provider. I'll check this branch out and have a look at what it's doing today
…com/pyansys/openapi-common into feat/222-add-client-credential-flow
|
Closing this, since the required use case is not standards-compliant, and we don't have a request for a standards-compliant implementation. |
2 participants
Closes #222
This PR includes a very rough initial implementation of the client credential flow from requests-auth:
with_oauth()method towith_oauth_pkce()with_oauth_client_credentials()I suspect there's a lot of duplication here, and that this isn't necessarily the most user-friendly way of doing it. Adding another level of builder also doesn't seem sensible though.
Tested against a real service implementing OIDC client credential flow and confirmed working.