feature: add client_auth_method=none into tokens for clients with empty secret #2504
Conversation
…ty secret e.g. cf client. This allows clients to see, whats really used. According to https://oauth.net/2.1/ the empty secret can be treated as public usage
…dClientAuthToEmptySecrets
empty is a valid authentication with empty secret, however it is no OAuth2 standard
…dClientAuthToEmptySecretsOwnMethod
…dClientAuthToEmptySecretsOwnMethod
…ty secret Internally use empty, but write none into tokens. Externally use always none, but internally empty to distinguish in the flow. The refresh flow with an empty secret is something we allow for now.
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186122537 The labels on this github issue will be updated when the story is started. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/RefreshRotationTest.java
Show resolved
Hide resolved
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/RefreshRotationTest.java
Show resolved
Hide resolved
- such that it's clearer what the test cases are
|
@strehle Based on the title & description of this PR, it seems that this PR's sole purpose is to inject an extra claim But when I look at all the tests added (I use the test changes to understand the exact implications of the code changes), it looks like the PR does more than that (for example, a test is added to forbid refresh request under some conditions)? Is that true? Or are these test "backfilled tests" testing existing logics of To help me understand this PR better, could you summarize, point by point, all the changes included in this PR. This will help me validate my understandings & help us find out what doc edits are required. |
The inject of And this blocks it right now, means the mix of the public refresh exchange with this approach to limit the empty secret. The empty secret and its refresh is an existing scenario which we cannot break. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServices.java
Show resolved
Hide resolved
strehle
changed the title
|
@peterhaochen47 please see this PR as replacement for #2435 |
|
I'm trying to understand the original problem (that this PR is intended to help address) better: #2193 (comment) |
This PR is needed to solve failing test b) , see #2193 (comment) because this change add client_auth_method=none to cf client autentication and then later we can use none to identiy that it was public and public-2-public should be ok |
…p' of github.com:cloudfoundry/uaa into feature/addClientAuthToNone4EmptySecrets
|
@peterhaochen47 After rebase this PR is smaller and therefore maybe you can now review this much faster ? |
|
Based on your comment, I thought that this PR was mainly for resolving the failures in #2193 which I have requested to be closed. Do you now suggest that this PR be reviewed independently without considering #2193? |
Yes please do so. I had the plan to re-use this, but we may wont fix user token. However this PR is simply about that none should appear in access_token for a not existing secret and for an empty one, so that we can treat in future both as public usage. |
|
FYI: this change is not related to #2571 even if topic is empty secret. |
Internally use empty, but write none into tokens. Externally use always none, but internally empty to distinguish in the flow.
The refresh flow with an empty secret is something we allow for now.
So a combination from #2435 (external view) and
#2447 (internal view)