consent: Resolve nil pointer panic in logout flow (#1418)

Closes #1403

Signed-off-by: aeneasr <aeneas@ory.sh>

consent/manager_sql.go

@@ -584,12 +584,16 @@ func (m *SQLManager) GetLogoutRequest(ctx context.Context, challenge string) (*L
 		return nil, sqlcon.HandleError(err)
 	}
 
-	c, err := m.r.ClientManager().GetConcreteClient(ctx, d.Client)
-	if err != nil {
-		return nil, err
+	if d.Client.Valid {
+		c, err := m.r.ClientManager().GetConcreteClient(ctx, d.Client.String)
+		if err != nil {
+			return nil, err
+		}
+
+		return d.ToLogoutRequest(c), nil
 	}
 
-	return d.ToLogoutRequest(c), nil
+	return d.ToLogoutRequest(nil), nil
 }
 
 func (m *SQLManager) VerifyAndInvalidateLogoutRequest(ctx context.Context, verifier string) (*LogoutRequest, error) {

consent/manager_test_helpers.go

@@ -92,7 +92,13 @@ func MockConsentRequest(key string, remember bool, rememberFor int, hasError boo
 	return c, h
 }
 
-func MockLogoutRequest(key string) (c *LogoutRequest) {
+func MockLogoutRequest(key string, withClient bool) (c *LogoutRequest) {
+	var cl *client.Client
+	if withClient {
+		cl = &client.Client{
+			ClientID: "fk-client-" + key,
+		}
+	}
 	return &LogoutRequest{
 		Subject:               "subject" + key,
 		Challenge:             "challenge" + key,
@@ -103,7 +109,7 @@ func MockLogoutRequest(key string) (c *LogoutRequest) {
 		PostLogoutRedirectURI: "http://redirect-me/",
 		WasUsed:               false,
 		Accepted:              false,
-		Client:                &client.Client{ClientID: "fk-client-" + key},
+		Client:                cl,
 	}
 }
 
@@ -622,19 +628,22 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 
 			t.Run("case=LogoutRequest", func(t *testing.T) {
 				for k, tc := range []struct {
-					key    string
-					authAt bool
+					key        string
+					authAt     bool
+					withClient bool
 				}{
-					{"LogoutRequest-1", true},
-					{"LogoutRequest-2", true},
-					{"LogoutRequest-3", true},
-					{"LogoutRequest-4", true},
-					{"LogoutRequest-5", true},
-					{"LogoutRequest-6", false},
+					{"LogoutRequest-1", true, true},
+					{"LogoutRequest-2", true, true},
+					{"LogoutRequest-3", true, true},
+					{"LogoutRequest-4", true, true},
+					{"LogoutRequest-5", true, false},
+					{"LogoutRequest-6", false, false},
 				} {
 					t.Run("key="+tc.key, func(t *testing.T) {
-						c := MockLogoutRequest(tc.key)
-						clientManager.CreateClient(context.TODO(), c.Client) // Ignore errors that are caused by duplication
+						c := MockLogoutRequest(tc.key, tc.withClient)
+						if tc.withClient {
+							clientManager.CreateClient(context.TODO(), c.Client) // Ignore errors that are caused by duplication
+						}
 
 						_, err := m.GetLogoutRequest(context.TODO(), "challenge"+tc.key)
 						require.Error(t, err)
@@ -679,7 +688,11 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
 }
 
 func compareLogoutRequest(t *testing.T, a, b *LogoutRequest) {
-	assert.EqualValues(t, a.Client.GetID(), b.Client.GetID())
+	require.True(t, (a.Client != nil && b.Client != nil) || (a.Client == nil && b.Client == nil))
+	if a.Client != nil {
+		assert.EqualValues(t, a.Client.GetID(), b.Client.GetID())
+	}
+
 	assert.EqualValues(t, a.Challenge, b.Challenge)
 	assert.EqualValues(t, a.Subject, b.Subject)
 	assert.EqualValues(t, a.Verifier, b.Verifier)

consent/migrations/sql/mysql/12.sql

@@ -0,0 +1,6 @@
+-- +migrate Up
+ALTER TABLE hydra_oauth2_logout_request MODIFY client_id varchar(255) NULL DEFAULT NULL;
+
+-- +migrate Down
+DELETE FROM hydra_oauth2_logout_request WHERE client_id IS NULL;
+ALTER TABLE hydra_oauth2_logout_request MODIFY client_id varchar(255) NOT NULL;

consent/migrations/sql/postgres/12.sql

@@ -0,0 +1,6 @@
+-- +migrate Up
+ALTER TABLE hydra_oauth2_logout_request ALTER COLUMN client_id DROP NOT NULL;
+
+-- +migrate Down
+DELETE FROM hydra_oauth2_logout_request WHERE client_id IS NULL;
+ALTER TABLE hydra_oauth2_logout_request ALTER COLUMN client_id SET NOT NULL;

consent/migrations/sql/tests/12_test.sql

@@ -0,0 +1,46 @@
+-- +migrate Up
+INSERT INTO hydra_client (id, allowed_cors_origins, client_name, client_secret, redirect_uris, grant_types, response_types, scope, owner, policy_uri, tos_uri, client_uri, logo_uri, contacts, client_secret_expires_at, sector_identifier_uri, jwks, jwks_uri, token_endpoint_auth_method, request_uris, request_object_signing_alg, userinfo_signed_response_alg, subject_type, audience, frontchannel_logout_uri, frontchannel_logout_session_required, post_logout_redirect_uris, backchannel_logout_uri, backchannel_logout_session_required)
+VALUES
+  ('12-client', 'http://localhost|http://google', 'some-client', 'abcdef', 'http://localhost|http://google', 'authorize_code|implicit', 'token|id_token', 'foo|bar', 'aeneas', 'http://policy', 'http://tos', 'http://client', 'http://logo', 'aeneas|foo', 0, 'http://sector', '{"keys": []}', 'http://jwks', 'none', 'http://uri1|http://uri2', 'rs256', 'rs526', 'public', 'https://www.ory.sh/api', 'http://fc-logout/', true, 'http://redir1/|http://redir2/', 'http://bc-logout/', true);
+
+INSERT INTO
+	hydra_oauth2_authentication_session (id, authenticated_at, subject, remember)
+VALUES
+	('12-login-session-id', NOW(), '12-sub', true);
+
+INSERT INTO
+	hydra_oauth2_authentication_request (challenge, verifier, client_id, subject, request_url, skip, requested_scope, csrf, authenticated_at, requested_at, oidc_context, login_session_id, requested_at_audience)
+VALUES
+	('12-challenge', '12-verifier', '12-client', '12-subject', '12-redirect', false, '12-scope', '12-csrf', NOW(), NOW(), '{}', '12-login-session-id', '12-aud');
+
+INSERT INTO
+	hydra_oauth2_consent_request (challenge, verifier, client_id, subject, request_url, skip, requested_scope, csrf, authenticated_at, requested_at, oidc_context, forced_subject_identifier, login_session_id, login_challenge, requested_at_audience, acr, context)
+VALUES
+	('12-challenge', '12-verifier', '12-client', '12-subject', '12-redirect', false, '12-scope', '12-csrf', NOW(), NOW(), '{}', '12-forced-sub', '12-login-session-id', '12-challenge', '12-aud', '12-acr', '{"foo":"bar"}');
+
+INSERT INTO
+	hydra_oauth2_consent_request_handled (challenge, granted_scope, remember, remember_for, error, requested_at, session_access_token, session_id_token, authenticated_at, was_used, granted_at_audience)
+VALUES
+	('12-challenge', '12-scope', true, 3600, '{}', NOW(), '{}', '{}', NOW(), false, '12-aud');
+
+INSERT INTO
+	hydra_oauth2_authentication_request_handled (challenge, subject, remember, remember_for, error, acr, requested_at, authenticated_at, was_used, forced_subject_identifier, context)
+VALUES
+	('12-challenge', '12-sub', true, 3600, '{}', '1', NOW(), NOW(), false, '12-forced-sub', '{"foo":"bar"}');
+
+INSERT INTO
+	hydra_oauth2_obfuscated_authentication_session (subject, client_id, subject_obfuscated)
+VALUES
+	('12-sub', '12-client', '12-obfuscated');
+
+INSERT INTO
+	hydra_oauth2_logout_request (challenge, verifier, subject, sid, client_id, request_url, redir_url, was_used, accepted, rejected, rp_initiated)
+VALUES
+	('12-challenge', '12-verifier', '12-subject', '12-session-id', '12-client', 'https://request-url/', 'https://redirect-url', false, false, false, false);
+
+INSERT INTO
+	hydra_oauth2_logout_request (challenge, verifier, subject, sid, client_id, request_url, redir_url, was_used, accepted, rejected, rp_initiated)
+VALUES
+	('12-1-challenge', '12-1-verifier', '12-1-subject', '12-1-session-id', NULL, 'https://request-url/', 'https://redirect-url', false, false, false, false);
+
+-- +migrate Down

consent/sdk_test.go

@@ -85,10 +85,10 @@ func TestSDK(t *testing.T) {
 	_, err = m.HandleConsentRequest(context.TODO(), "challenge3", hcr3)
 	require.NoError(t, err)
 
-	lur1 := MockLogoutRequest("testsdk-1")
+	lur1 := MockLogoutRequest("testsdk-1", true)
 	require.NoError(t, m.CreateLogoutRequest(context.TODO(), lur1))
 
-	lur2 := MockLogoutRequest("testsdk-2")
+	lur2 := MockLogoutRequest("testsdk-2", false)
 	require.NoError(t, m.CreateLogoutRequest(context.TODO(), lur2))
 
 	crGot, err := sdk.Admin.GetConsentRequest(admin.NewGetConsentRequestParams().WithConsentChallenge("challenge1"))

consent/sql_helper.go

@@ -126,20 +126,28 @@ var sqlParamsLogoutRequest = []string{
 }
 
 type sqlLogoutRequest struct {
-	Challenge             string `db:"challenge"`
-	Verifier              string `db:"verifier"`
-	Subject               string `db:"subject"`
-	SessionID             string `db:"sid"`
-	RequestURL            string `db:"request_url"`
-	PostLogoutRedirectURI string `db:"redir_url"`
-	WasUsed               bool   `db:"was_used"`
-	Accepted              bool   `db:"accepted"`
-	Rejected              bool   `db:"rejected"`
-	Client                string `db:"client_id"`
-	RPInitiated           bool   `db:"rp_initiated"`
+	Challenge             string         `db:"challenge"`
+	Verifier              string         `db:"verifier"`
+	Subject               string         `db:"subject"`
+	SessionID             string         `db:"sid"`
+	RequestURL            string         `db:"request_url"`
+	PostLogoutRedirectURI string         `db:"redir_url"`
+	WasUsed               bool           `db:"was_used"`
+	Accepted              bool           `db:"accepted"`
+	Rejected              bool           `db:"rejected"`
+	Client                sql.NullString `db:"client_id"`
+	RPInitiated           bool           `db:"rp_initiated"`
 }
 
 func newSQLLogoutRequest(c *LogoutRequest) *sqlLogoutRequest {
+	var clientID sql.NullString
+	if c.Client != nil {
+		clientID = sql.NullString{
+			Valid:  true,
+			String: c.Client.ClientID,
+		}
+	}
+
 	return &sqlLogoutRequest{
 		Challenge:             c.Challenge,
 		Verifier:              c.Verifier,
@@ -149,7 +157,7 @@ func newSQLLogoutRequest(c *LogoutRequest) *sqlLogoutRequest {
 		PostLogoutRedirectURI: c.PostLogoutRedirectURI,
 		WasUsed:               c.WasUsed,
 		Accepted:              c.Accepted,
-		Client:                c.Client.ClientID,
+		Client:                clientID,
 		RPInitiated:           c.RPInitiated,
 	}
 }