Reorder IDP form elements

[charliepark]

So far, this branch is only modifying the create form, though once feedback is in, I'll modify the update form to match.

This addresses #2471, where Augustus proposed updating the IDP form to more logically group the various components. The main changes here are …

• adding headers for the various sections
• reordering the fields
• adding a checkbox for signed requests, which then reveals the public cert / private key fields

Here's the updated layout …

… and with the "Signed requests" box checked …

I'm open to any other modifications; just opening this up for discussion and moving the ball down the field.

Closes #2471

[vercel]

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

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Nov 6, 2024 4:57pm

[david-crespo]

Looks good. Noticed the section headings are the same size as the modal title, which seems too big, but I see you followed Firewall Rules, which is the same way. They're all text-sans-2xl (1.5625rem, so I tried text-sans-xl (1.125rem) and it's too small, which is presumably how I landed on 2xl for the sections. Need @benjaminleonard's input here — is there/should there be a heading in between those sizes?

[benjaminleonard]

I'm trying to recall if we're using headings for any pane forms currently. It's a reasonable addition but there are broader implications in terms of rolling it out to every form that chunks the contents like this.

This might be a reason to move this into a full-page like the instance create form. What do you think? That might give us space to add some more inventive UI also.

[benjaminleonard]

Just noticing you mentioned firewall rules - will explore what headings should look like there. I didn't have them in the original design but they are helpful. Not exactly sure what the distinction is but firewall rule is not something that should be converted to a full form right.

[david-crespo]

Yeah, the firewall rules form probably feels better in the context of the list. It would be a little jarring on its own page. The main problem with the form is the length, so I'm not sure a full page form would help much anyway, though I guess you could do something like putting target type and value pickers side by side. I wonder if there's room for a wider side modal form.

[charliepark]

Still hoping for some font-size guidance on that 1.5xl size font (Ben is checking it out), but as this is closer, I'm going to take it out of draft mode.

[david-crespo]

Looking great. I think we can figure out the headers later. If this went out in v12 with the big headers, I'd be ok with it.

This should be tweaked, though:

• I think the checkbox should be checked by default as nearly everyone in production will want to include certs (confirm @augustuswm?)
• (Optional) on the checkbox doesn't make sense, the checkbox is not optional, it is always either checked or unchecked

Really I'm thinking we can probably do without the checkbox since the fields will nearly always want to be used, and they're already optional anyway.

[david-crespo]

Not suggesting a change here, just musing — the linked table is pretty good. It's a shame the generated doc for the endpoint is not nearly as helpful. It's kind of hard to see how it could be as good, since the table includes hand-written notes about, e.g., how the name will be used in context. I wonder if there's a way we could add our own notes in the API structs in addition to the default Name docs. Unfortunately these comes from another struct:

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct SamlIdentityProviderCreate {
    #[serde(flatten)]
    pub identity: IdentityMetadataCreateParams,

[augustuswm]

I think it is actually the inverse. I would expect more installs to not use signed requests than those that would. I definitely don't think we have the intention of making it mandatory as I would not be surprised to encounter IdPs that do not support signed requests. I think it is good to have it checked by default to encourage the use of signed requests. In terms of validation, the main caveat is that it if signed requests are to be used a private key must be supplied. It shouldn't be possible to submit a request certificate without a key. As for the request certificate I need to look at how we expose it via the control plane to determine if it is mandatory given a key. The safe case is to say that if one is present then the other must be as well.

The inverse of all this (signed responses) are mandatory and fundamental to SAML, but that is handled by the IdP metadata url / xml.

[david-crespo]

Ah, ok! I was thinking of responses. If we want to encourage it, I think it’s ok to leave the checkbox out and just validate the fields together so they error if they’re empty while the other is not. Maybe mention the rule in the help text on each?

[david-crespo]

new headings look great, thanks @benjaminleonard

[david-crespo]

Followup on validating the keypair: #1564 (comment) (we already had an issue for it, sort of). Current state is worse than I thought.

[augustuswm]

Thank you for all the work on this. It looks so much better and I think a lot more streamlined.

One note looking at the preview again. Without the checkbox for the cert / key pair we need some kind of header to explain what they are for. I think the most appropriate header would be "Request Signing"

[david-crespo]

Good idea, will add.