-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: Add support for requesting an audience to the OAuth2 Introspection pr… #678
feat: Add support for requesting an audience to the OAuth2 Introspection pr… #678
Conversation
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.
Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)
@@ -265,11 +266,18 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth | |||
var rt http.RoundTripper | |||
|
|||
if c.PreAuth != nil && c.PreAuth.Enabled { | |||
var ep url.Values | |||
|
|||
if c.PreAuth.Audience != "" { |
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.
Could you please add a test for this? :)
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.
Thanks! I can do that. However, there are no tests currently for this (for example for scopes), and I'm not sure how to write it.
I added this:
index 2487b09..dba83d0 100644
--- a/pipeline/authn/authenticator_oauth2_introspection_test.go
+++ b/pipeline/authn/authenticator_oauth2_introspection_test.go
@@ -474,6 +474,45 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
},
expectErr: false,
},
+ {
+ d: "should pass because audience and scopes match configuration",
+ r: &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+ config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true,"scope":["foo","bar"]} }`),
+ setup: func(t *testing.T, m *httprouter.Router) {
+ m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+ require.NoError(t, r.ParseForm())
+ require.Equal(t, "", r.Form.Get("audience"))
+ require.Equal(t, "foo bar", r.Form.Get("scope"))
+ })
+ },
+ expectErr: false,
+ },
+ {
+ d: "should pass because audience and scopes match configuration",
+ r: &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+ config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true,"scope":["foo","bar"]} }`),
+ setup: func(t *testing.T, m *httprouter.Router) {
+ m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+ require.NoError(t, r.ParseForm())
+ require.Equal(t, "", r.Form.Get("audience"))
+ require.Equal(t, "foo bar", r.Form.Get("scope"))
+ })
+ },
+ expectErr: false,
+ },
+ {
+ d: "should pass because audience and scopes should not be requested",
+ r: &http.Request{Header: http.Header{"Authorization": {"bearer token"}}},
+ config: []byte(`{ "pre_authorization":{"client_id":"some_id","client_secret":"some_secret","enabled":true} }`),
+ setup: func(t *testing.T, m *httprouter.Router) {
+ m.POST("/oauth2/token", func(_ http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+ require.NoError(t, r.ParseForm())
+ require.Equal(t, "", r.Form.Get("audience"))
+ require.Equal(t, "", r.Form.Get("scope"))
+ })
+ },
+ expectErr: false,
+ },
} {
t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) {
router := httprouter.New()
@@ -484,6 +523,8 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
defer ts.Close()
tc.config, _ = sjson.SetBytes(tc.config, "introspection_url", ts.URL+"/oauth2/introspect")
+ tc.config, _ = sjson.SetBytes(tc.config, "pre_authorization.token_url", ts.URL+"/oauth2/token")
+
sess := new(AuthenticationSession)
err := a.Authenticate(tc.r, sess, tc.config, nil)
if tc.expectErr {
Which fails with:
--- FAIL: TestAuthenticatorOAuth2Introspection (4.42s)
--- FAIL: TestAuthenticatorOAuth2Introspection/method=authenticate (4.40s)
--- FAIL: TestAuthenticatorOAuth2Introspection/method=authenticate/case=24/description=should_pass_because_audience_and_scopes_match_configuration (1.64s)
authenticator_oauth2_introspection_test.go:536:
Error Trace: authenticator_oauth2_introspection_test.go:536
Error: Received unexpected error:
Post "http://127.0.0.1:36911/oauth2/introspect": unexpected end of JSON input
github.com/ory/oathkeeper/pipeline/authn.(*AuthenticatorOAuth2Introspection).Authenticate
/home/user/oathkeeper/pipeline/authn/authenticator_oauth2_introspection.go:194
github.com/ory/oathkeeper/pipeline/authn_test.TestAuthenticatorOAuth2Introspection.func1.23
/home/user/oathkeeper/oathkeeper/pipeline/authn/authenticator_oauth2_introspection_test.go:529
testing.tRunner
/usr/lib/go-1.16/src/testing/testing.go:1194
runtime.goexit
/usr/lib/go-1.16/src/runtime/asm_amd64.s:1371
Test: TestAuthenticatorOAuth2Introspection/method=authenticate/case=24/description=should_pass_because_audience_and_scopes_match_configuration
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.
If you expect introspection to pass, you need to add a JSON payload to the response along the lines of: https://github.com/ory/oathkeeper/blob/master/pipeline/authn/authenticator_oauth2_introspection_test.go#L462-L474
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.
Done!
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.
Awesome, thank you! 🎉 Your contribution makes Ory better :)
Related issue
#677
Proposed changes
Adding an option to request an audience, and adding the audience to the preauth request.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments