Automatic ACS URL in IdP forms w/ copy button

[charliepark]

https://console-git-automaticacsurl-oxidecomputer.vercel.app/system/silos/maze-war/idps-new

This adds a click-to-copy component as an option for inputs, and then uses that new feature in the IDP form to allow the user to copy an auto-generated ACS URL.

Augustus had the very good idea (in #2472):

When creating a SAML identity provider connection, the ACS URL that the user enters should always be of the form:

http(s)://<silo>.sys.<subdomain>/login/<silo>/saml/<idp>

where is the Silo name, is the subdomain assigned to the rack, and is the name of the IdP connection.

This PR, then, sets up that form to have the ACS value automatically generated.

Closes #2472

---

There are a two things I have questions on.

1. I think the visual treatment of the "copy" button could use a little tweaking, but I've had a really hard time getting Tailwind to cooperate. Specifically, giving the button (or a wrapper div for it) a left-border and possibly a different background color.
2. fieldProps.value as string … would rather not cast it, but none of the other things I tried seemed to handle it.

[vercel]

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

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Nov 8, 2024 3:56am

[david-crespo]

value has a funky type because it gets it from the native input, but we could override it to just be string:

diff --git a/app/ui/lib/TextInput.tsx b/app/ui/lib/TextInput.tsx
index 786c1ee6..5e64bcef 100644
--- a/app/ui/lib/TextInput.tsx
+++ b/app/ui/lib/TextInput.tsx
@@ -8,6 +8,7 @@
 import { announce } from '@react-aria/live-announcer'
 import cn from 'classnames'
 import React, { useEffect } from 'react'
+import type { Merge } from 'type-fest'
 
 import { CopyToClipboard } from './CopyToClipboard'
 
@@ -34,14 +35,18 @@ export type TextAreaProps =
 // it makes a bunch of props required that should be optional. Instead we simply
 // take the props of an input field (which are part of the Field props) and
 // manually tack on validate.
-export type TextInputBaseProps = React.ComponentPropsWithRef<'input'> & {
+export type TextInputBaseProps = Merge<
+  React.ComponentPropsWithRef<'input'>,
+  {
     // error is used to style the wrapper, also to put aria-invalid on the input
     error?: boolean
     disabled?: boolean
     className?: string
     fieldClassName?: string
     copyable?: boolean
+    value?: string
   }
+>
 
 export const TextInput = React.forwardRef<
   HTMLInputElement,
@@ -88,7 +93,7 @@ export const TextInput = React.forwardRef<
         />
         {copyable && (
           <CopyToClipboard
-            text={fieldProps.value as string}
+            text={fieldProps.value}
             className="flex h-full items-stretch border-l border-solid px-3 border-default hover:border-hover"
           />
         )}

It doesn't seem like we're passing anything other than string to it anywhere because there are no type errors when I do that, except text is still unhappy because it's not allowed to be undefined.

$ ts
app/ui/lib/TextInput.tsx:96:13 - error TS2322: Type 'string | undefined' is not assignable to type 'string'.
  Type 'undefined' is not assignable to type 'string'.

96             text={fieldProps.value}
               ~~~~

  app/ui/lib/CopyToClipboard.tsx:19:3
    19   text: string
         ~~~~
    The expected type comes from property 'text' which is declared here on type 'IntrinsicAttributes & Props'


Found 1 error in app/ui/lib/TextInput.tsx:96

In this particular case, we know it won't be undefined because we're constructing it and passing it in, but the only way to ensure that in the code is something like

{copyable && fieldProps.value && <CopyToClipboard />}

Which I guess is fine, though it risks us accidentally doing something weird later where we set copyable but don't pass in a value or pass in a falsy value. Maybe a slightly stronger check would be fieldProps.value !== undefined.

Another option would be to not use a real input at all here, and instead construct a little div that looks like an input, then we don't have to modify TextInput to support this.

[david-crespo]

Kinda sad that you can't see the URL (especially the part that changes when you type the IdP name) without copying it and pasting it somewhere but I guess that's ok. Not sure what visual treatment would let you see the whole thing.

[charliepark]

Tooltip?

[charliepark]

[charliepark]

As discussed in chat … using a full-height button and making the entire area clickable:

Eh … it's a little too narrow; will widen it a smidge.

[charliepark]

Now matches the other dropdown buttons:

[charliepark]

I noticed that the combobox border was running full-height, like the number field / copyable field …

… so I updated it to reflect the regular Listbox style, with the top and bottom gap …

[david-crespo]

We don't have an e2e test exercising the IdP create form! I will add one.

[david-crespo]

This is awesome. It works really well. The only thing I wanna do before merging is make sure there’s really no way the user can be accessing the console through some other domain (through a proxy, maybe) that could cause us to be wrong to pull the delegated domain out of window.location.

[david-crespo]

Based on some discussion in chat, it seems like my concern about a proxy is founded.

If the proxy is doing TLS termination and initiating a new TLS connection to us, then yes, I think what you're describing would be a problem.

The idea would be that there is a "true" rack hostname that the IdP is supposed to use when talking to the rack, but the user is hitting the rack through a proxy that gives them a different one, so putting the latter in the ACS URL would be wrong. This is only a problem if the IdP is not also supposed to be using the same proxy. This only makes sense if the proxy is doing TLS termination because otherwise it could not change the hostname on the incoming request, and Nexus would reject it because Nexus relies on the hostname being the one it knows about.

This all seems kind of unlikely, so one option is to do nothing and see if it becomes a problem. If it is, the user can always use the CLI instead. I don't love that because even though it's unlikely, it's a wrinkle that pops up at rack setup time, which is an especially critical time for customer perception of the product.

Options

1. Don't change anything. In the unlikely event this comes up, tell the user to use the CLI to create the IdP, and then fix the console UI in the next release based on what we learn about the use case.
2. Start with the field disabled and auto-generated, have add a checkbox or edit button under it that would allow an override if necessary.
   • If checkbox: when unchecked we just make it a regular text field, i.e., don't touch it when the IdP name changes. If they unchecked it and checked it again, it would have to revert back to the generated version
3. Don't disable the field, auto-generate it only as long as the field has not been touched
   • This is bad because it's not clear about when it's being auto-generated vs. not.

Any other ways we could do it?

[charliepark]

Open to copy suggestions on both (1) microcopy above ACS URL input and (2) checkbox label

[david-crespo]

I'm thinking we can overexplain in the help text and that will let us keep the checkbox label short. I'm also realizing that "Service provider" is a lot less clear than saying what it really means, which is the rack. Maybe something more like:

Oxide endpoint for the identity provider to send the SAML response. URL is generated from the current hostname, silo name, and provider name according to a standard format.

And then what if the checkbox says "Override standard format" and defaults to unchecked? That part isn’t necessary, it could still be “Use standard format” and default checked, like you have it.

[charliepark]

[david-crespo]

This works, but it would be simpler as a regular useState and a Checkbox instead of a CheckboxField

[david-crespo]

Awesome. Try the useState thing, see if it’s nicer, but this is good to go.