Skip to content
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.

Sign up for GitHub

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

Improves compatibility with OIDC Conformity Tests #873

Merged
merged 2 commits into from
May 24, 2018
Merged

Improves compatibility with OIDC Conformity Tests #873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f96bc4
oauth2: Rejects requests without nonce in implicit/hybrid
May 23, 2018
ac10ae1
consent: Requests re-permission only custom schemes are used
May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,9 @@
name = "github.com/oleiade/reflections"
version = "1.0.0"

[[constraint]]
name = "github.com/ory/dockertest"
version = "3.3.0"

[[constraint]]
name = "github.com/ory/fosite"
version = "0.19.4"
version = "0.19.5"

[[constraint]]
name = "github.com/ory/graceful"
Expand Down
34 changes: 28 additions & 6 deletions consent/strategy_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,14 +377,37 @@ func (s *DefaultStrategy) requestConsent(w http.ResponseWriter, r *http.Request,
return s.forwardConsentRequest(w, r, ar, authenticationSession, nil)
}

// https://tools.ietf.org/html/rfc6749
//
// As stated in Section 10.2 of OAuth 2.0 [RFC6749], the authorization
// server SHOULD NOT process authorization requests automatically
// without user consent or interaction, except when the identity of the
// client can be assured. This includes the case where the user has
// previously approved an authorization request for a given client id --
// unless the identity of the client can be proven, the request SHOULD
// be processed as if no previous request had been approved.
//
// Measures such as claimed "https" scheme redirects MAY be accepted by
// authorization servers as identity proof. Some operating systems may
// offer alternative platform-specific identity features that MAY be
// accepted, as appropriate.
if ar.GetClient().IsPublic() {
return s.forwardConsentRequest(w, r, ar, authenticationSession, nil)
// The OpenID Connect Test Tool fails if this returns `consent_required` when `prompt=none` is used.
// According to the quote above, it should be ok to allow https to skip consent.
//
// This is tracked as issue: https://github.com/ory/hydra/issues/866
// This is also tracked as upstream issue: https://github.com/openid-certification/oidctest/issues/97
if ar.GetRedirectURI().Scheme != "https" {
return s.forwardConsentRequest(w, r, ar, authenticationSession, nil)
}
}

if ar.GetResponseTypes().Has("token") {
// We're probably requesting the implicit or hybrid flow in which case we MUST authenticate and authorize the request
return s.forwardConsentRequest(w, r, ar, authenticationSession, nil)
}
// This breaks OIDC Conformity Tests and is probably a bit paranoid.
//
// if ar.GetResponseTypes().Has("token") {
// // We're probably requesting the implicit or hybrid flow in which case we MUST authenticate and authorize the request
// return s.forwardConsentRequest(w, r, ar, authenticationSession, nil)
// }

consentSessions, err := s.M.FindPreviouslyGrantedConsentRequests(ar.GetClient().GetID(), authenticationSession.Subject)
if errors.Cause(err) == errNoPreviousConsentFound {
Expand All @@ -406,7 +429,6 @@ func (s *DefaultStrategy) forwardConsentRequest(w http.ResponseWriter, r *http.R
skip = true
}

// Let'id validate that prompt is actually not "none" if we can't skip authentication
prompt := stringsx.Splitx(ar.GetRequestForm().Get("prompt"), " ")
if stringslice.Has(prompt, "none") && !skip {
return errors.WithStack(fosite.ErrConsentRequired.WithDebug(`Prompt "none" was requested, but no previous consent was found`))
Expand Down
146 changes: 82 additions & 64 deletions consent/strategy_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"net/http"
"net/http/cookiejar"
"net/http/httptest"
"net/url"
"testing"
"time"

Expand All @@ -53,6 +54,12 @@ func mustRSAKey() *rsa.PrivateKey {
return key
}

func mustParseURL(t *testing.T, u string) *url.URL {
uu, err := url.Parse(u)
require.NoError(t, err)
return uu
}

func mockProvider(h *func(w http.ResponseWriter, r *http.Request)) *httptest.Server {
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
(*h)(w, r)
Expand Down Expand Up @@ -301,67 +308,78 @@ func TestStrategy(t *testing.T) {
},
},
{
d: "This should pass but require consent because it's not an authorization_code flow",
req: fosite.AuthorizeRequest{ResponseTypes: fosite.Arguments{"token", "code", "id_token"}, Request: fosite.Request{Client: &client.Client{ID: "client-id"}, Scopes: []string{"scope-a"}}},
jar: persistentCJ,
lph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
rr, res, err := apiClient.GetLoginRequest(r.URL.Query().Get("login_challenge"))
require.NoError(t, err)
require.EqualValues(t, http.StatusOK, res.StatusCode)
assert.True(t, rr.Skip)
assert.Equal(t, "user", rr.Subject)

v, res, err := apiClient.AcceptLoginRequest(r.URL.Query().Get("login_challenge"), swagger.AcceptLoginRequest{
Subject: "user",
Remember: false,
RememberFor: 0,
Acr: "1",
})
require.NoError(t, err)
require.EqualValues(t, http.StatusOK, res.StatusCode)
require.NotEmpty(t, v.RedirectTo)
http.Redirect(w, r, v.RedirectTo, http.StatusFound)
}
},
cph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
rr, res, err := apiClient.GetConsentRequest(r.URL.Query().Get("consent_challenge"))
require.NoError(t, err)
require.EqualValues(t, http.StatusOK, res.StatusCode)
assert.False(t, rr.Skip)
assert.Equal(t, "client-id", rr.Client.Id)
assert.Equal(t, "user", rr.Subject)

v, res, err := apiClient.AcceptConsentRequest(r.URL.Query().Get("consent_challenge"), swagger.AcceptConsentRequest{
GrantScope: []string{"scope-a"},
Remember: false,
RememberFor: 0,
Session: swagger.ConsentRequestSession{
AccessToken: map[string]interface{}{"foo": "bar"},
IdToken: map[string]interface{}{"bar": "baz"},
},
})
require.NoError(t, err)
require.EqualValues(t, http.StatusOK, res.StatusCode)
require.NotEmpty(t, v.RedirectTo)
http.Redirect(w, r, v.RedirectTo, http.StatusFound)
}
},
expectFinalStatusCode: http.StatusOK,
expectErrType: []error{ErrAbortOAuth2Request, ErrAbortOAuth2Request, nil},
expectErr: []bool{true, true, false},
expectSession: &HandledConsentRequest{
ConsentRequest: &ConsentRequest{Subject: "user"},
GrantedScope: []string{"scope-a"},
Remember: false,
RememberFor: 0,
Session: &ConsentRequestSessionData{
AccessToken: map[string]interface{}{"foo": "bar"},
IDToken: map[string]interface{}{"bar": "baz"},
},
},
d: "This should fail because prompt=none, client is public, and redirection scheme is not HTTPS but a custom scheme",
req: fosite.AuthorizeRequest{RedirectURI: mustParseURL(t, "custom://redirection-scheme/path"), Request: fosite.Request{Client: &client.Client{Public: true, ID: "client-id"}, Scopes: []string{"scope-a"}}},
prompt: "none",
jar: persistentCJ,
lph: passAuthentication(apiClient, false),
expectFinalStatusCode: fosite.ErrConsentRequired.StatusCode(),
expectErrType: []error{ErrAbortOAuth2Request, fosite.ErrConsentRequired},
expectErr: []bool{true, true},
},
// This test is disabled because it breaks OIDC Conformity Tests
//{
// d: "This should pass but require consent because it's not an authorization_code flow",
// req: fosite.AuthorizeRequest{ResponseTypes: fosite.Arguments{"token", "code", "id_token"}, Request: fosite.Request{Client: &client.Client{ID: "client-id"}, Scopes: []string{"scope-a"}}},
// jar: persistentCJ,
// lph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) {
// return func(w http.ResponseWriter, r *http.Request) {
// rr, res, err := apiClient.GetLoginRequest(r.URL.Query().Get("login_challenge"))
// require.NoError(t, err)
// require.EqualValues(t, http.StatusOK, res.StatusCode)
// assert.True(t, rr.Skip)
// assert.Equal(t, "user", rr.Subject)
//
// v, res, err := apiClient.AcceptLoginRequest(r.URL.Query().Get("login_challenge"), swagger.AcceptLoginRequest{
// Subject: "user",
// Remember: false,
// RememberFor: 0,
// Acr: "1",
// })
// require.NoError(t, err)
// require.EqualValues(t, http.StatusOK, res.StatusCode)
// require.NotEmpty(t, v.RedirectTo)
// http.Redirect(w, r, v.RedirectTo, http.StatusFound)
// }
// },
// cph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) {
// return func(w http.ResponseWriter, r *http.Request) {
// rr, res, err := apiClient.GetConsentRequest(r.URL.Query().Get("consent_challenge"))
// require.NoError(t, err)
// require.EqualValues(t, http.StatusOK, res.StatusCode)
// assert.False(t, rr.Skip)
// assert.Equal(t, "client-id", rr.Client.Id)
// assert.Equal(t, "user", rr.Subject)
//
// v, res, err := apiClient.AcceptConsentRequest(r.URL.Query().Get("consent_challenge"), swagger.AcceptConsentRequest{
// GrantScope: []string{"scope-a"},
// Remember: false,
// RememberFor: 0,
// Session: swagger.ConsentRequestSession{
// AccessToken: map[string]interface{}{"foo": "bar"},
// IdToken: map[string]interface{}{"bar": "baz"},
// },
// })
// require.NoError(t, err)
// require.EqualValues(t, http.StatusOK, res.StatusCode)
// require.NotEmpty(t, v.RedirectTo)
// http.Redirect(w, r, v.RedirectTo, http.StatusFound)
// }
// },
// expectFinalStatusCode: http.StatusOK,
// expectErrType: []error{ErrAbortOAuth2Request, ErrAbortOAuth2Request, nil},
// expectErr: []bool{true, true, false},
// expectSession: &HandledConsentRequest{
// ConsentRequest: &ConsentRequest{Subject: "user"},
// GrantedScope: []string{"scope-a"},
// Remember: false,
// RememberFor: 0,
// Session: &ConsentRequestSessionData{
// AccessToken: map[string]interface{}{"foo": "bar"},
// IDToken: map[string]interface{}{"bar": "baz"},
// },
// },
//},
{
d: "This should fail at login screen because subject from accept does not match subject from session",
req: fosite.AuthorizeRequest{ResponseTypes: fosite.Arguments{"code"}, Request: fosite.Request{Client: &client.Client{ID: "client-id"}, Scopes: []string{"scope-a"}}},
Expand Down Expand Up @@ -976,20 +994,21 @@ func TestStrategy(t *testing.T) {
cph = noopHandler(t)
}

calls := 0
calls := -1
aph = func(w http.ResponseWriter, r *http.Request) {
calls++
require.True(t, len(tc.expectErrType) >= calls+1, "%d (expect) < %d (got)", len(tc.expectErrType), calls+1)
require.True(t, len(tc.expectErr) >= calls+1, "%d (expect) < %d (got)", len(tc.expectErr), calls+1)
require.NoError(t, r.ParseForm())
tc.req.Form = r.Form

c, err := strategy.HandleOAuth2AuthorizationRequest(w, r, &tc.req)
t.Logf("DefaultStrategy returned:\n\t%+v\n\t%s", c, err)
t.Logf("DefaultStrategy returned at call %d:\n\tgot: %+v\n\texpected: %s", calls, c, err)

if tc.expectErr[calls] {
assert.Error(t, err)
if tc.expectErrType[calls] != nil {
assert.EqualError(t, err, tc.expectErrType[calls].Error(), "%+v", err)
assert.EqualError(t, tc.expectErrType[calls], err.Error(), "%+v", err)
}
} else {
require.NoError(t, err)
Expand All @@ -1002,7 +1021,6 @@ func TestStrategy(t *testing.T) {
}
}

calls++
if errors.Cause(err) == ErrAbortOAuth2Request {
// nothing to do, indicates redirect
} else if err != nil {
Expand Down