Add Group Attribute Name field to IdP view

[charliepark]

This field was already present, just hidden in the "edit" (though not really editable) view.

I also adjusted the help text's CSS to have text-balance, so we avoid having ragged descriptions.

Closes #2473

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Oct 28, 2024 9:17pm

[david-crespo]

I don't think they're actually comma-separated. It's all XML.

<saml:AttributeStatement>
  <saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">
    <saml:AttributeValue>SRE</saml:AttributeValue>
    <saml:AttributeValue>Admins</saml:AttributeValue>
  </saml:Attribute>
</saml:AttributeStatement>

https://github.com/oxidecomputer/omicron/blob/5bf05de207bf05201aea7c07f24ddc7724707008/nexus/tests/integration_tests/data/saml_response_with_groups.xml#L94-L98

Maybe we can be a little less concrete about the format anyway. It doesn't matter here:

Name of the SAML attribute in the IdP response listing the user's groups

We could go even shorter to get it on one line, but I don't think this is consistent with our more typically breezy style.

Name of SAML attribute in IdP response listing user's groups

Worth noting that text-balance looks worse and worse the shorter the thing gets. I'd take it back out.

The docs also say "comma-separated". I will make a note to confirm it's wrong and fix it.

[david-crespo]

Docs fix: https://github.com/oxidecomputer/docs/pull/484

[jmpesp]

It can be a single AttributeStatement, like you posted, but it can also be multple:

      <saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">
        <saml:AttributeValue>Admins</saml:AttributeValue>
      </saml:Attribute>
      <saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">
        <saml:AttributeValue>SRE</saml:AttributeValue>
      </saml:Attribute>

the Keycloak setting for this is (under the "groups" mapping):

It can also be a comma separated list of groups, if the IDP sends that. Some applications apparently do require that (eg: https://support.okta.com/help/s/question/0D54z00009Jau72CAB/group-attribute-statements-combine-all-groups-into-a-comma-separated-string?language=en_US)

[david-crespo]

Very helpful, thanks. Do all of those work the same way with respect to the attribute name? In which case just not mentioning the details seems best.

[jmpesp]

Do all of those work the same way with respect to the attribute name? In which case just not mentioning the details seems best.

I believe so yeah, both use the same Attribute Name="groups", and it'd be a bug not to. Agree on eliding that detail yeah. Even so if we ever encounter an IDP that only sends comma separated lists Nexus can handle it.

[david-crespo]

The create form has a field for this too — its description should match. Yet another case where it would be convenient to share the fields (and in this case it would have helped catch this mismatch) but it feels kinda boilerplatey to get the types right on shared stuff. We should think about how to make it easier, or if we can't, maybe at least commit to a particular pattern for sharing fields and document it so we don't have to think about it. Out of scope for this PR of course.

console/app/forms/idp/create.tsx

Lines 133 to 138 in 4e96756

	<TextField
	name="groupAttributeName"
	label="Group attribute name"
	description="Name of SAML attribute where we can find a comma-separated list of names of groups the user belongs to"
	control={form.control}
	/>

[david-crespo]

Not a blocker, but I'd recommend adding groupAttributeName to the mock SAML IdP so we can see it in the form.

console/mock-api/silo.ts

Lines 76 to 88 in 4e96756

	export const samlIdp: Json<SamlIdentityProvider> = {
	id: '2a96ce6f-c178-4631-9cde-607d65b539c7',
	description: 'An identity provider but what if it had a really long description',
	name: 'mock-idp',
	time_created: new Date(2021, 4, 3, 4).toISOString(),
	time_modified: new Date(2021, 4, 3, 5).toISOString(),
	acs_url: '',
	idp_entity_id: '',
	public_cert: '',
	slo_url: '',
	sp_client_id: '',
	technical_contact_email: '',
	}

Might also be worth adding a line alongside this asserting that it shows up. Don't use the text= way, do it the expect(page.getByRole('textbox', { name: 'Group attribute name' })).toHaveValue('groups') way.

console/test/e2e/silos.e2e.ts

Lines 181 to 187 in 4e96756

	await expectVisible(page, [
	'role=dialog[name="Identity provider"]',
	'role=heading[name="mock-idp"]',
	// random stuff that's not in the table
	'text="Entity ID"',
	'text="Single Logout (SLO) URL"',
	])

[david-crespo]

I just did it since the comment was 95% of it.