Fix user scope propagations in tokens #761
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
XVincentX
force-pushed
the
bugfix/user-scope-propagation
branch
from
6ace6d7 to
0319031
Compare
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
==========================================
- Coverage 88.93% 88.92% -0.01%
==========================================
Files 129 129
Lines 3543 3532 -11
==========================================
- Hits 3151 3141 -10
+ Misses 392 391 -1
Continue to review full report at Codecov.
|
XVincentX
force-pushed
the
bugfix/user-scope-propagation
branch
from
79ab2c5 to
960339d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following PR will fix the scope propagation story around the oAuth2 story.
What (I think) was wrong
Basically I noticed that all the scope resolution and checking was done on the
oauth2credential type, which is the one that is merely identifying the client.Consider the following scenario:
oAuth2credential type (identifying an application). I put on this credential the scopesread,write.basic-authcredential set, and I put diffetent scopes, such ascreate,investigateread/writescopes, although my personal credential have a different set of scopesThis PR is changing this behavior putting the user scope at the center of the checks and returned values.
What (I think) was the intention
Now that I recall also other changes I made in the identity server, I think the original intention was to have user-level
oauth2credentials; this thing was unfortunately not documented and not implemented correctly — that has been causing a lot of confusion that led us to discover these bugs.In other words — we still need to work a lot on the credential system, but we'll slowly get there somehow. 🤞
Other perks
authorizeCredential— that means all and nothing — tocheckScopesOnCredential, which is what that function is doing for real.transaction_idheader in the oAuth2 response, which is not required by the spec (it took me ages to discover why)ifstatements around thecheckCredentialScopesfunction; it's got already an inner check that's returningtruein case the scopes aren't provided.This needs to be reviewed very carefully as it's changing some internal behaviors that I think they were wrong. The only concern I have — I might be missing some of the original intentions so please guys, point anything that feels weird to you.
Connect #740
Closes #740
Needs #758