diff --git a/consent/handler.go b/consent/handler.go index 71fbec3020..6af2c3c387 100644 --- a/consent/handler.go +++ b/consent/handler.go @@ -334,11 +334,11 @@ func (h *Handler) AcceptLoginRequest(w http.ResponseWriter, r *http.Request, ps return } - if !ar.Skip { - p.AuthenticatedAt = time.Now().UTC() - } else { - p.Remember = false + if ar.Skip { + p.Remember = true // If skip is true remember is also true to allow consecutive calls as the same user! p.AuthenticatedAt = ar.AuthenticatedAt + } else { + p.AuthenticatedAt = time.Now().UTC() } p.RequestedAt = ar.RequestedAt diff --git a/consent/manager_memory.go b/consent/manager_memory.go index 867f976e8b..9a89e0b3b0 100644 --- a/consent/manager_memory.go +++ b/consent/manager_memory.go @@ -354,7 +354,7 @@ func (m *MemoryManager) ConfirmLoginSession(ctx context.Context, id string, subj m.m["authSessions"].Lock() defer m.m["authSessions"].Unlock() if c, ok := m.authSessions[id]; ok { - c.Remember = true + c.Remember = remember c.Subject = subject c.AuthenticatedAt = time.Now().UTC() m.authSessions[id] = c diff --git a/consent/strategy_default.go b/consent/strategy_default.go index e15ccf20be..4ef1512ecf 100644 --- a/consent/strategy_default.go +++ b/consent/strategy_default.go @@ -349,8 +349,8 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re return nil, err } - if session.LoginRequest.Skip && session.Remember { - return nil, errors.WithStack(fosite.ErrServerError.WithDebug("The login request is marked as remember, but is also marked as skipped - only one of the values can be true.")) + if session.LoginRequest.Skip && !session.Remember { + return nil, errors.WithStack(fosite.ErrServerError.WithDebug("The login request was previously remembered and can only be forgotten using the reject feature.")) } if session.LoginRequest.Skip && session.Subject != session.LoginRequest.Subject { diff --git a/consent/strategy_default_test.go b/consent/strategy_default_test.go index 3bf544824a..9440e42f04 100644 --- a/consent/strategy_default_test.go +++ b/consent/strategy_default_test.go @@ -777,6 +777,74 @@ func TestStrategyLoginConsent(t *testing.T) { }, }, }, + { + d: "This should pass because login and consent have been granted, this time we remember the decision", + req: fosite.AuthorizeRequest{Request: fosite.Request{Client: &client.Client{ClientID: "client-id"}, RequestedScope: []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) { + res, err := apiClient.Admin.GetLoginRequest(admin.NewGetLoginRequestParams().WithLoginChallenge(r.URL.Query().Get("login_challenge"))) + require.NoError(t, err) + require.True(t, res.Payload.Skip) + passAuthentication(apiClient, true)(t)(w, r) + } + }, + cph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + rrr, err := apiClient.Admin.GetConsentRequest(admin.NewGetConsentRequestParams().WithConsentChallenge(r.URL.Query().Get("consent_challenge"))) + require.NoError(t, err) + require.True(t, rrr.Payload.Skip) + passAuthorization(apiClient, true)(t)(w, r) + } + }, + expectFinalStatusCode: http.StatusOK, + expectErrType: []error{ErrAbortOAuth2Request, ErrAbortOAuth2Request, nil}, + expectErr: []bool{true, true, false}, + expectSession: &HandledConsentRequest{ + ConsentRequest: &ConsentRequest{Subject: "user", SubjectIdentifier: "user"}, + GrantedScope: []string{"scope-a"}, + Remember: true, + RememberFor: 0, + Session: &ConsentRequestSessionData{ + AccessToken: map[string]interface{}{"foo": "bar"}, + IDToken: map[string]interface{}{"bar": "baz"}, + }, + }, + }, + { + d: "This should pass because login and consent have been granted, this time we remember the decision", + req: fosite.AuthorizeRequest{Request: fosite.Request{Client: &client.Client{ClientID: "client-id"}, RequestedScope: []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) { + res, err := apiClient.Admin.GetLoginRequest(admin.NewGetLoginRequestParams().WithLoginChallenge(r.URL.Query().Get("login_challenge"))) + require.NoError(t, err) + require.True(t, res.Payload.Skip) + passAuthentication(apiClient, true)(t)(w, r) + } + }, + cph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + rrr, err := apiClient.Admin.GetConsentRequest(admin.NewGetConsentRequestParams().WithConsentChallenge(r.URL.Query().Get("consent_challenge"))) + require.NoError(t, err) + require.True(t, rrr.Payload.Skip) + passAuthorization(apiClient, true)(t)(w, r) + } + }, + expectFinalStatusCode: http.StatusOK, + expectErrType: []error{ErrAbortOAuth2Request, ErrAbortOAuth2Request, nil}, + expectErr: []bool{true, true, false}, + expectSession: &HandledConsentRequest{ + ConsentRequest: &ConsentRequest{Subject: "user", SubjectIdentifier: "user"}, + GrantedScope: []string{"scope-a"}, + Remember: true, + RememberFor: 0, + Session: &ConsentRequestSessionData{ + AccessToken: map[string]interface{}{"foo": "bar"}, + IDToken: map[string]interface{}{"bar": "baz"}, + }, + }, + }, { d: "This should pass because login was remembered and session id should be set and session context should also work", req: fosite.AuthorizeRequest{Request: fosite.Request{Client: &client.Client{ClientID: "client-id"}, RequestedScope: []string{"scope-a"}}}, @@ -818,22 +886,7 @@ func TestStrategyLoginConsent(t *testing.T) { assert.NotEmpty(t, cr.LoginChallenge) assert.Equal(t, map[string]interface{}{"foo": "bar"}, cr.Context) - vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). - WithConsentChallenge(r.URL.Query().Get("consent_challenge")). - WithBody(&models.HandledConsentRequest{ - GrantedScope: []string{"scope-a"}, - Remember: false, - RememberFor: 0, - Session: &models.ConsentRequestSessionData{ - AccessToken: map[string]interface{}{"foo": "bar"}, - IDToken: map[string]interface{}{"bar": "baz"}, - }, - })) - require.NoError(t, err) - v := vr.Payload - - require.NotEmpty(t, v.RedirectTo) - http.Redirect(w, r, v.RedirectTo, http.StatusFound) + passAuthorization(apiClient, false)(t)(w, r) } }, expectFinalStatusCode: http.StatusOK, @@ -1023,22 +1076,7 @@ func TestStrategyLoginConsent(t *testing.T) { assert.Equal(t, "user", rr.Subject) assert.Empty(t, rr.Client.Secret) - vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). - WithConsentChallenge(r.URL.Query().Get("consent_challenge")). - WithBody(&models.HandledConsentRequest{ - GrantedScope: []string{"scope-a"}, - Remember: false, - RememberFor: 0, - Session: &models.ConsentRequestSessionData{ - AccessToken: map[string]interface{}{"foo": "bar"}, - IDToken: map[string]interface{}{"bar": "baz"}, - }, - })) - require.NoError(t, err) - v := vr.Payload - - require.NotEmpty(t, v.RedirectTo) - http.Redirect(w, r, v.RedirectTo, http.StatusFound) + passAuthorization(apiClient, false)(t)(w, r) } }, expectFinalStatusCode: http.StatusOK, @@ -1155,22 +1193,7 @@ func TestStrategyLoginConsent(t *testing.T) { rr := rrr.Payload assert.True(t, rr.Skip) - vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). - WithConsentChallenge(r.URL.Query().Get("consent_challenge")). - WithBody(&models.HandledConsentRequest{ - GrantedScope: []string{"scope-a"}, - Remember: false, - RememberFor: 0, - Session: &models.ConsentRequestSessionData{ - AccessToken: map[string]interface{}{"foo": "bar"}, - IDToken: map[string]interface{}{"bar": "baz"}, - }, - })) - require.NoError(t, err) - v := vr.Payload - - require.NotEmpty(t, v.RedirectTo) - http.Redirect(w, r, v.RedirectTo, http.StatusFound) + passAuthorization(apiClient, false)(t)(w, r) } }, expectFinalStatusCode: http.StatusOK, @@ -1275,22 +1298,7 @@ func TestStrategyLoginConsent(t *testing.T) { rr := rrr.Payload assert.False(t, rr.Skip) - vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). - WithConsentChallenge(r.URL.Query().Get("consent_challenge")). - WithBody(&models.HandledConsentRequest{ - GrantedScope: []string{"scope-a"}, - Remember: false, - RememberFor: 0, - Session: &models.ConsentRequestSessionData{ - AccessToken: map[string]interface{}{"foo": "bar"}, - IDToken: map[string]interface{}{"bar": "baz"}, - }, - })) - require.NoError(t, err) - v := vr.Payload - - require.NotEmpty(t, v.RedirectTo) - http.Redirect(w, r, v.RedirectTo, http.StatusFound) + passAuthorization(apiClient, false)(t)(w, r) } }, expectFinalStatusCode: http.StatusOK, diff --git a/oauth2/oauth2_auth_code_test.go b/oauth2/oauth2_auth_code_test.go index 26e65c35bd..094b14a6df 100644 --- a/oauth2/oauth2_auth_code_test.go +++ b/oauth2/oauth2_auth_code_test.go @@ -91,6 +91,56 @@ type clientCreator interface { CreateClient(cxt context.Context, client *hc.Client) error } +func acceptLogin(apiClient *hydra.OryHydra, subject string, expectSkip bool, expectSubject string) func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + rrr, err := apiClient.Admin.GetLoginRequest(admin.NewGetLoginRequestParams().WithLoginChallenge(r.URL.Query().Get("login_challenge"))) + require.NoError(t, err) + + rr := rrr.Payload + assert.Equal(t, expectSkip, rr.Skip) + assert.EqualValues(t, expectSubject, rr.Subject) + + vr, err := apiClient.Admin.AcceptLoginRequest(admin.NewAcceptLoginRequestParams(). + WithLoginChallenge(r.URL.Query().Get("login_challenge")). + WithBody(&models.HandledLoginRequest{Subject: pointerx.String(subject)})) + require.NoError(t, err) + + v := vr.Payload + require.NotEmpty(t, v.RedirectTo) + http.Redirect(w, r, v.RedirectTo, http.StatusFound) + } + } +} + +func acceptConsent(apiClient *hydra.OryHydra, scope []string, expectSkip bool, expectSubject string) func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + rrr, err := apiClient.Admin.GetConsentRequest(admin.NewGetConsentRequestParams().WithConsentChallenge(r.URL.Query().Get("consent_challenge"))) + require.NoError(t, err) + + rr := rrr.Payload + assert.Equal(t, expectSkip, rr.Skip) + assert.EqualValues(t, expectSubject, rr.Subject) + + vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). + WithConsentChallenge(r.URL.Query().Get("consent_challenge")). + WithBody(&models.HandledConsentRequest{ + GrantedScope: scope, + Session: &models.ConsentRequestSessionData{ + AccessToken: map[string]interface{}{"foo": "bar"}, + IDToken: map[string]interface{}{"bar": "baz"}, + }, + })) + require.NoError(t, err) + v := vr.Payload + + require.NotEmpty(t, v.RedirectTo) + http.Redirect(w, r, v.RedirectTo, http.StatusFound) + } + } +} + // TestAuthCodeWithDefaultStrategy runs proper integration tests against in-memory and database connectors, specifically // we test: // @@ -685,49 +735,8 @@ func TestAuthCodeWithDefaultStrategy(t *testing.T) { }, authURL: oauthConfig.AuthCodeURL("some-hardcoded-state") + "&prompt=none&max_age=60", cj: persistentCJ, - lph: func(t *testing.T) func(w http.ResponseWriter, r *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - rrr, err := apiClient.Admin.GetLoginRequest(admin.NewGetLoginRequestParams().WithLoginChallenge(r.URL.Query().Get("login_challenge"))) - require.NoError(t, err) - - rr := rrr.Payload - assert.True(t, rr.Skip) - assert.EqualValues(t, "user-a", rr.Subject) - - vr, err := apiClient.Admin.AcceptLoginRequest(admin.NewAcceptLoginRequestParams(). - WithLoginChallenge(r.URL.Query().Get("login_challenge")). - WithBody(&models.HandledLoginRequest{Subject: pointerx.String("user-a")})) - require.NoError(t, err) - - v := vr.Payload - 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) { - rrr, err := apiClient.Admin.GetConsentRequest(admin.NewGetConsentRequestParams().WithConsentChallenge(r.URL.Query().Get("consent_challenge"))) - require.NoError(t, err) - rr := rrr.Payload - assert.True(t, rr.Skip) - assert.EqualValues(t, "user-a", rr.Subject) - - vr, err := apiClient.Admin.AcceptConsentRequest(admin.NewAcceptConsentRequestParams(). - WithConsentChallenge(r.URL.Query().Get("consent_challenge")). - WithBody(&models.HandledConsentRequest{ - GrantedScope: []string{"hydra", "offline"}, - Session: &models.ConsentRequestSessionData{ - AccessToken: map[string]interface{}{"foo": "bar"}, - IDToken: map[string]interface{}{"bar": "baz"}, - }, - })) - require.NoError(t, err) - v := vr.Payload - - require.NotEmpty(t, v.RedirectTo) - http.Redirect(w, r, v.RedirectTo, http.StatusFound) - } - }, + lph: acceptLogin(apiClient, "user-a", true, "user-a"), + cph: acceptConsent(apiClient, []string{"hydra", "offline"}, true, "user-a"), cb: func(t *testing.T) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { code = r.URL.Query().Get("code") @@ -784,6 +793,57 @@ func TestAuthCodeWithDefaultStrategy(t *testing.T) { expectOAuthAuthError: true, expectOAuthTokenError: false, }, + { + d: "checks if consecutive authentications cause any issues (1/3)", + authURL: oauthConfig.AuthCodeURL("some-hardcoded-state") + "&prompt=none&max_age=60", + cj: persistentCJ, + lph: acceptLogin(apiClient, "user-a", true, "user-a"), + cph: acceptConsent(apiClient, []string{"hydra", "offline"}, true, "user-a"), + expectOAuthAuthError: false, + expectOAuthTokenError: false, + expectRefreshToken: true, + cb: func(t *testing.T) httprouter.Handle { + return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + code = r.URL.Query().Get("code") + require.NotEmpty(t, code) + w.WriteHeader(http.StatusOK) + } + }, + }, + { + d: "checks if consecutive authentications cause any issues (2/3)", + authURL: oauthConfig.AuthCodeURL("some-hardcoded-state") + "&prompt=none&max_age=60", + cj: persistentCJ, + lph: acceptLogin(apiClient, "user-a", true, "user-a"), + cph: acceptConsent(apiClient, []string{"hydra", "offline"}, true, "user-a"), + expectOAuthAuthError: false, + expectOAuthTokenError: false, + expectRefreshToken: true, + cb: func(t *testing.T) httprouter.Handle { + return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + code = r.URL.Query().Get("code") + require.NotEmpty(t, code) + w.WriteHeader(http.StatusOK) + } + }, + }, + { + d: "checks if consecutive authentications cause any issues (3/3)", + authURL: oauthConfig.AuthCodeURL("some-hardcoded-state") + "&prompt=none&max_age=60", + cj: persistentCJ, + lph: acceptLogin(apiClient, "user-a", true, "user-a"), + cph: acceptConsent(apiClient, []string{"hydra", "offline"}, true, "user-a"), + expectOAuthAuthError: false, + expectOAuthTokenError: false, + expectRefreshToken: true, + cb: func(t *testing.T) httprouter.Handle { + return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + code = r.URL.Query().Get("code") + require.NotEmpty(t, code) + w.WriteHeader(http.StatusOK) + } + }, + }, { d: "checks if authenticatedAt/requestedAt is properly forwarded across the lifecycle by checking if prompt=none fails when maxAge is reached", authURL: oauthConfig.AuthCodeURL("some-hardcoded-state") + "&prompt=none&max_age=1",