chore: include merged claims into the database #15570

Emyrk · 2024-11-18T16:58:50Z

Merging happens before IDP sync. Storing this will make some SQL queries much simplier.

Currently the SQL queries look like this:

coder/coderd/database/queries/user_links.sql

Lines 65 to 97 in 664e734

    
           SELECT 
        
           	DISTINCT jsonb_object_keys(claims->'id_token_claims') 
        
           FROM 
        
           	user_links 
        
           WHERE 
        
               -- Only return rows where the top level key exists 
        
           	claims ? 'id_token_claims' AND 
        
               -- 'null' is the default value for the id_token_claims field 
        
           	-- jsonb 'null' is not the same as SQL NULL. Strip these out. 
        
           	jsonb_typeof(claims->'id_token_claims') != 'null' AND 
        
           	login_type = 'oidc' 
        
           	AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid  THEN 
        
           		user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id) 
        
           		ELSE true 
        
           	END 
        
           -- Merge with user_info claims. 
        
           UNION 
        
            -- This query is identical to the one above, except for 'user_info_claims'. 
        
            -- There might be some way to do this more concisely at a cost of readability. 
        
           SELECT 
        
           	DISTINCT jsonb_object_keys(claims->'user_info_claims') 
        
           FROM 
        
           	user_links 
        
           WHERE 
        
           	claims ? 'user_info_claims' AND 
        
           	jsonb_typeof(claims->'user_info_claims') != 'null' AND 
        
           	login_type = 'oidc' 
        
           	AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid  THEN 
        
           		user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id) 
        
           		ELSE true 
        
           	END

Query is duplicated over both fields with a union. This query is also incorrect, as merged claims do not properly merge arrays of duplicated fields.

Merging happens before IDP sync. Storing this will make some SQL queries much simplier

johnstcn

Should this not also require updated tests also in coderd/idpsync?

Emyrk · 2024-11-18T17:42:33Z

The real tests are in this PR: #15525

There is only a debug endpoint that returns this information today, and I did not want to add tests for a debug endpoint. So it requires more effort than it's worth imo to fully test this feature outside of the PRs I have in flight.

Writing the queries to use this data, I realized merging the fields simplifies a lot and makes claims more correct.

johnstcn

Stamp 👍 in combination with test improoooovements in #15525

chore: include merged claims into the database

d1fb4eb

Merging happens before IDP sync. Storing this will make some SQL queries much simplier

github-actions bot assigned Emyrk Nov 18, 2024

update unit test

47d2c76

johnstcn reviewed Nov 18, 2024

View reviewed changes

johnstcn approved these changes Nov 18, 2024

View reviewed changes

Emyrk merged commit 4fedc7c into main Nov 18, 2024
32 checks passed

Emyrk deleted the stevenmasley/idp_field_include_merged branch November 18, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: include merged claims into the database #15570

chore: include merged claims into the database #15570

Emyrk commented Nov 18, 2024 •

edited

Loading

johnstcn left a comment

Emyrk commented Nov 18, 2024 •

edited

Loading

johnstcn left a comment

	SELECT
	DISTINCT jsonb_object_keys(claims->'id_token_claims')
	FROM
	user_links
	WHERE
	-- Only return rows where the top level key exists
	claims ? 'id_token_claims' AND
	-- 'null' is the default value for the id_token_claims field
	-- jsonb 'null' is not the same as SQL NULL. Strip these out.
	jsonb_typeof(claims->'id_token_claims') != 'null' AND
	login_type = 'oidc'
	AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
	user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id)
	ELSE true
	END

	-- Merge with user_info claims.
	UNION

	-- This query is identical to the one above, except for 'user_info_claims'.
	-- There might be some way to do this more concisely at a cost of readability.
	SELECT
	DISTINCT jsonb_object_keys(claims->'user_info_claims')
	FROM
	user_links
	WHERE
	claims ? 'user_info_claims' AND
	jsonb_typeof(claims->'user_info_claims') != 'null' AND
	login_type = 'oidc'
	AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
	user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id)
	ELSE true
	END

chore: include merged claims into the database #15570

chore: include merged claims into the database #15570

Conversation

Emyrk commented Nov 18, 2024 • edited Loading

johnstcn left a comment

Choose a reason for hiding this comment

Emyrk commented Nov 18, 2024 • edited Loading

johnstcn left a comment

Choose a reason for hiding this comment

Emyrk commented Nov 18, 2024 •

edited

Loading

Emyrk commented Nov 18, 2024 •

edited

Loading