-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cross clients improvement - requesting client ID always added to the audience claim #1088
Cross clients improvement - requesting client ID always added to the audience claim #1088
Conversation
.travis.yml
Outdated
@@ -1,4 +1,5 @@ | |||
language: go | |||
go_import_path: github.com/coreos/dex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this, it doesn't have anything to do with the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. I added go_import_path in travis.yaml because I was not able to run tests for my fork in Travis. With go_import_path Travis should work for your project as well as for all forks
server/server_test.go
Outdated
@@ -21,12 +21,12 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
oidc "github.com/coreos/go-oidc" | |||
"github.com/coreos/go-oidc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't rewrite these imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
server/server_test.go
Outdated
oauth2Server *httptest.Server | ||
} | ||
|
||
type crossClientsTestDeferred struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these new types? I'm confused by the naming here. Can you add some explanations or not refactor the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced new types because there were cyclic dependencies in this code, and I wanted to somehow refactor it. But I will remove this refactoring as you suggested in overall comment, and I will just duplicate this test for the new case.
server/server_test.go
Outdated
@@ -690,10 +690,14 @@ func TestOAuth2ImplicitFlow(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestCrossClientScopes(t *testing.T) { | |||
func setupCrossClientsFixture(t *testing.T, crossClientsTest func(*crossClientsFixture, *crossClientsTestDeferred) (reqDump, respDump []byte)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does crossClientsTest
do? What should it perform? Why does it return reqDump
and respDump
? Can we just avoid this re-factor in this PR?
server/server_test.go
Outdated
Endpoint: p.Endpoint(), | ||
Scopes: []string{ | ||
oidc.ScopeOpenID, "profile", "email", | ||
"audience:server:client_id:" + client.ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we just remove this claim for the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to ensure backward-compatibility so I left an old test (if clientID is explicitly requested, issued ID token should still contain only one occurrence of clientID in audience claim)
Thanks. Overall I think I'd rather avoid doing any test refactoring in this PR. They need refactoring but that's a bigger job than just changing the claim behavior. Can you squash your commits before you push again? |
lgtm! Please squash |
933c351
to
e3c9b49
Compare
Squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi @rithujohn191
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks
…lue is an array of case sensitive strings." (https://openid.net/specs/openid-connect-core-1_0.html#IDToken) If one specifies "cross-client trust" (https://github.com/dexidp/dex/blob/master/Documentation/custom-scopes-claims-clients.md#cross-client-trust-and-authorized-party - I think the example in the Dex-Doc where the "ID token claims" are shown is not correct any more), an array is returned for "Aud" (due to dexidp/dex#1088).
…e-claim-fix Cross clients improvement - requesting client ID always added to the audience claim
See #1087