From ac10ae16dda48814352366faa5e10ff8d3f3aab0 Mon Sep 17 00:00:00 2001 From: arekkas Date: Thu, 24 May 2018 11:30:08 +0200 Subject: [PATCH] consent: Requests re-permission only custom schemes are used See #866 --- consent/strategy_default.go | 34 +++++-- consent/strategy_default_test.go | 146 +++++++++++++++++-------------- 2 files changed, 110 insertions(+), 70 deletions(-) diff --git a/consent/strategy_default.go b/consent/strategy_default.go index 244ec35863..026c5ab7e1 100644 --- a/consent/strategy_default.go +++ b/consent/strategy_default.go @@ -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 { @@ -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`)) diff --git a/consent/strategy_default_test.go b/consent/strategy_default_test.go index 7db92ba508..9f1b89c56f 100644 --- a/consent/strategy_default_test.go +++ b/consent/strategy_default_test.go @@ -28,6 +28,7 @@ import ( "net/http" "net/http/cookiejar" "net/http/httptest" + "net/url" "testing" "time" @@ -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) @@ -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"}}}, @@ -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) @@ -1002,7 +1021,6 @@ func TestStrategy(t *testing.T) { } } - calls++ if errors.Cause(err) == ErrAbortOAuth2Request { // nothing to do, indicates redirect } else if err != nil {