Add token create forms to Join Tokens UI by avatus · Pull Request #44408 · gravitational/teleport

avatus · 2024-07-18T18:35:57Z

This adds the create tokens forms to our Join Tokens UI. The forms only support IAM, GCP and tokens for now (preferred) and allow a yaml editor for other types. You can also edit IAM and GCP and "token" in yaml if you wish but its purposefully kinda "hidden".

This adds the feature to the navigation under "Join Tokens".

closes #31671

kimlisa

some other UX issues:

when the dialog is open (viewing a token), if i press the create button or try viewing other tokens, my current opened dialog does not update the state (btw you can trigger a component re-mount by defining a key attribute)
when i delete the token that i am viewing, it does not close the dialog

kimlisa · 2024-07-19T21:30:21Z

The forms only support IAM, GCP and tokens for now (preferred) and allow a yaml editor for other types

unless i'm missing something, i can't access the yaml editor until after I am editing something 🤔

avatus · 2024-07-19T21:32:47Z

We don't allow a YAML editor to create at all and push towards creating only IAM or GCP. So you are correct that you will only see it while editing

avatus · 2024-07-22T23:28:11Z

had to rebase, sorry. new commit here 3aa99c9

ryanclark · 2024-07-23T08:47:42Z

+	tokenId := r.Header.Get(HeaderTokenName)
+	if tokenId == "" {
+		return nil, trace.BadParameter("requires a token name to edit")
+	}


Does this make the endpoint not backwards compatible? Should this use a new endpoint instead?

These are new endpoints (I'm editing this one but it isn't actually used/released yet, just from my last PR) so no worry about backward compatibility here.

ryanclark · 2024-07-23T08:48:02Z

-	h.POST("/webapi/token", h.WithAuth(h.createTokenHandle))
+	h.PUT("/webapi/token/yaml", h.WithAuth(h.editTokenYAML))
+	// used for creating a new token
+	h.POST("/webapi/token/new", h.WithAuth(h.upsertTokenHandle))


Should we have /new in the URL? I don't see any other endpoint that uses this convention

I can think of something better than /new, sure

i just made it plural "/webapi/tokens", as posting to a REST api should end in the plural of the resource anyway.

ryanclark · 2024-07-23T08:48:40Z

-	AllowRules []string `json:"allowRules,omitempty"`
+	// Allow is a list of allow rules
+	Allow []*types.TokenRule             `json:"allow,omitempty"`
+	GCP   *types.ProvisionTokenSpecV2GCP `json:"gcp,omitempty"`


Missing godoc

ryanclark · 2024-07-23T08:49:58Z

+const joinRoleOptions: Option<JoinRole, JoinRole>[] = [
+  'App',
+  'Node',
+  'Db',
+  'Kube',
+  'Bot',
+  'WindowsDesktop',
+  'Discovery',
+].map(role => ({ value: role as JoinRole, label: role as JoinRole }));
+
+const availableJoinMethods: Option<JoinMethod, JoinMethod>[] = [
+  'iam',
+  'gcp',
+].map(method => ({
+  value: method as JoinMethod,
+  label: method as JoinMethod,
+}));


Should the labels for these options be something a bit more user friendly?

I chose this to match what's in the documentation and YAML for these join methods. I think in this case particularly this would be the most friendly/understood.

ryanclark · 2024-07-23T08:51:47Z

+              </ButtonIcon>
+            </HoverTooltip>
+            <Text typography="h3" fontWeight={400}>
+              {editToken ? `Edit Token` : 'Create a New Join Token'}


Here the button would be "Create a New Join Token" and elsewhere there's a button that's "Create new token" - we should be consistent on capitalisation

ryanclark · 2024-07-23T08:52:00Z

+                editTokenWithYAML(editToken.id);
+              }}
+            >
+              <Text color="buttons.link.default">Use YAML editor instead</Text>


Maybe "instead" isn't needed here

I wanted to imply "one or the other" here but, i'll just remove "instead" and see if any user complains. thank you!

ryanclark · 2024-07-23T08:54:29Z

-          cluster. This will remove the ability for any new resources to join
-          with this token and any non-renewable resource from renewing.
+          cluster. This will remove the ability for any new resources to join or
+          renew with this token and any non-renewable resource from renewing.


"to join or renew with this token" - the "or renew with this token" part sounds a bit weird to me

I'll remove as "renew" is kind of a join anyway

ryanclark · 2024-07-23T08:57:15Z

+  setTokenState,
+}: {
+  rules: NewJoinTokenGCPState[];
+  setTokenState: React.Dispatch<React.SetStateAction<NewJoinTokenState>>;


I've never really been a fan of passing the React.Dispatch method to child components - I think components should care about themselves and offer a onXXX prop, then the parent component updates its own state instead of passing down the setState method

https://medium.com/@christopherthai/why-you-shouldnt-pass-react-s-setstate-as-a-prop-a-deep-dive-8a3dcd74bec8

Great point, will change!

ryanclark · 2024-07-23T08:58:31Z

+  padding-top: ${props => props.theme.space[3]}px;
+  padding-bottom: ${props => props.theme.space[3]}px;
+  padding-left: ${props => props.theme.space[3]}px;
+  padding-right: ${props => props.theme.space[3]}px;


Suggested change

padding-top: ${props => props.theme.space[3]}px;

padding-bottom: ${props => props.theme.space[3]}px;

padding-left: ${props => props.theme.space[3]}px;

padding-right: ${props => props.theme.space[3]}px;

padding: ${props => props.theme.space[3]}px;

ryanclark · 2024-07-25T12:34:14Z

+	// used for creating a new token
+	h.POST("/webapi/tokens", h.WithAuth(h.upsertTokenHandle))
+	// used for updating a token
+	h.PUT("/webapi/token", h.WithAuth(h.upsertTokenHandle))


Normally a REST API would be something like PUT /webapi/token/name for editing, and not rely on an arbitrary header for specifying the token name - thoughts?

We've chosen to do the header for tokens to obfuscate a potentially secret token name from request logs

ryanclark · 2024-07-25T12:36:26Z

+    const newRules = [...rules].filter((rule, i) => {
+      if (index === i) {
+        return false;
+      }
+      return rule;
+    });


Suggested change

const newRules = [...rules].filter((rule, i) => {

if (index === i) {

return false;

}

return rule;

});

const newRules = rules.filter((rule, i) => index !== i);

filter expects a boolean so we should be returning true or false, and .filter creates a new copy of the array so there's no need for [...rules]

Ah I think this was originally a map and then a filter and then map and then ended up with this monstrosity. Thanks for the catch

ryanclark · 2024-07-25T12:37:34Z

+      ))}
+      <ButtonText onClick={addNewRule}>
+        <Plus size={16} mr={2} />
+        Add another AWS Account


Should this be "Add another AWS rule" or something?

ryanclark · 2024-07-25T12:37:48Z

+    const newRules = [...rules].filter((rule, i) => {
+      if (index === i) {
+        return false;
+      }
+      return rule;
+    });


Same as above

ryanclark · 2024-07-25T12:38:33Z

+              updateRuleField(index, 'locations', opts as OptionGCP[])
+            }
+            value={rule.locations}
+            label="Add Locatons"


Suggested change

label="Add Locatons"

label="Add Locations"

ryanclark · 2024-07-25T12:39:48Z

+            {index > 0 && ( // at least one rule is required, so lets not allow the user to remove it
+              <ButtonIcon onClick={() => removeRule(index)}>
+                <Trash size={16} color="text.muted" />
+              </ButtonIcon>
+            )}


Same comment about rules.length > 1

ryanclark · 2024-07-25T12:40:00Z

+        >
+          <Flex alignItems="center" justifyContent="space-between">
+            <Text fontWeight={700} mb={2}>
+              AWS Account


"AWS Rule"?

ryanclark · 2024-07-25T12:40:34Z

+          <FieldInput
+            label="Account ID"
+            rule={requiredField('Account ID is required')}
+            placeholder="aws account ID"


Suggested change

placeholder="aws account ID"

placeholder="AWS Account ID"

ryanclark · 2024-07-25T12:43:23Z

+  'Discovery',
+].map(role => ({ value: role as JoinRole, label: role as JoinRole }));
+
+const availableJoinMethods: Option<JoinMethod, JoinMethod>[] = [


As these are typed as OptionJoinMethod and OptionJoinRole below, this should probably use those types

ryanclark · 2024-07-25T12:46:36Z

+            }
+            value={rule.project_ids}
+            label="Add Project ID(s)"
+            rule={requiredField('At least 1 projectID required')}


Suggested change

rule={requiredField('At least 1 projectID required')}

rule={requiredField('At least 1 project ID required')}

avatus · 2024-07-25T22:15:22Z

I will backport this in a group with the rest of the previous supporting PRs

kimlisa

overall lgtm, there is one change that will make creating integrations break that needs to be addressed

kimlisa · 2024-07-26T08:41:00Z

+	h.PUT("/webapi/token/yaml", h.WithAuth(h.editTokenYAML))
+	// used for creating a new token
+	h.POST("/webapi/tokens", h.WithAuth(h.upsertTokenHandle))
+	// used for updating a token
+	h.PUT("/webapi/token", h.WithAuth(h.upsertTokenHandle))


should all these be /webapi/tokens (plura)?

kimlisa · 2024-07-26T08:41:15Z

 	// join token handlers
-	h.PUT("/webapi/token/yaml", h.WithAuth(h.upsertTokenContent))
-	h.POST("/webapi/token", h.WithAuth(h.createTokenHandle))
+	h.PUT("/webapi/token/yaml", h.WithAuth(h.editTokenYAML))


nit. the UI tends use the word edit but backend uses update (afaik)

Suggested change

h.PUT("/webapi/token/yaml", h.WithAuth(h.editTokenYAML))

h.PUT("/webapi/token/yaml", h.WithAuth(h.updateTokenYAML))

kimlisa · 2024-07-26T08:44:50Z

 	}

+	if tokenId != extractedRes.Metadata.Name {
+		return nil, trace.BadParameter("renaming tokens is not supported.")


can't remember... i think error messaging only has proper period stuff if the msg starts with a capital

Suggested change

return nil, trace.BadParameter("renaming tokens is not supported.")

return nil, trace.BadParameter("renaming tokens is not supported")

kimlisa · 2024-07-26T08:47:08Z

+	}
+
+	if editing && tokenId != req.Name {
+		return nil, trace.BadParameter("renaming tokens is not supported.")


Suggested change

return nil, trace.BadParameter("renaming tokens is not supported.")

return nil, trace.BadParameter("renaming tokens is not supported")

kimlisa · 2024-07-26T08:48:40Z

+	expires := time.Now().UTC().Add(defaults.NodeJoinTokenTTL)
+	// IAM and GCP tokens should never expire
+	if req.JoinMethod == types.JoinMethodGCP || req.JoinMethod == types.JoinMethodIAM {
+		expires = time.Now().UTC().AddDate(1000, 0, 0)


is there a const for 1000? to consistently set a "never expires"

We could add a constant but the time package doesn't have Year (at least my editor doesn't seem to think so) so it'd end up being only a const of 1000 which might not get across that its a "time" so probably best to leave out

kimlisa · 2024-07-26T09:30:28Z


-  async function handleSave(content: string): Promise<void> {
-    const token = await ctx.joinTokenService.upsertJoinToken({ content });
+  function updateTokenList(token: JoinToken): JoinToken[] {


does line 97 still apply? (since we prevent user from changing the name on edit now)

kimlisa · 2024-07-26T09:31:33Z

+            labelTip="Allows regions and/or zones."
+          />
+          <FieldSelectCreatable
+            placeholder="PROJECT_NUMBER_compute@developer.gserviceaccount.com"


maybe a shorter email example? b/c i can't see the entire text in the input box

I think its in a specific format according to the docs, but i'll remove _NUMBER and that'll shorten it enough i think

kimlisa · 2024-07-26T09:33:17Z

+  padding: ${props => props.theme.space[3]}px;
+`;
+
+const JoinTokenIAMForm = ({


nit: i would pull these forms (JoinTokenIAMForm and JoinTokenGCPForm) out into its own file?

kimlisa · 2024-07-26T09:38:45Z

+          />
+          <FieldInput
+            label="ARN"
+            toolTipContent={`The joining nodes must match this ARN. Supports wildcards "*" and "?"`}


does this need to be a specific role arn format? can we maybe provide example? like how do you use the question mark in the arn?

eg (this is IAM role arn format)

arn:aws:iam::account-id:role/role-name

arn:aws:iam::account-id:role/*

question mark?

kimlisa · 2024-07-26T09:46:10Z

rename to UpsertXXX maybe? since it does both create and edit

ryanclark

Can you add some tests?

ryanclark · 2024-07-26T14:17:13Z

+            )}
+          </Flex>
+          <FieldSelectCreatable
+            placeholder="Type a project ID"


We should be consistent on capitalisation (project ID vs Project ID in the required rule)

avatus · 2024-07-26T15:06:13Z

I'll add more to the sorry now that the storybook was updated and some tests

ryanclark

LGTM. might be worth adding some more tests in the future around the rule configuration

avatus · 2024-07-26T17:00:40Z

might be worth adding some more tests in the future around the rule configuration

true, added some more around adding/deleting rules and also updating select join roles

avatus · 2024-07-26T17:26:42Z

Rebased. @kimlisa the feedback for your comments starts with 8f919a3

avatus added the no-changelog Indicates that a PR does not require a changelog entry label Jul 18, 2024

avatus requested review from kimlisa and rudream July 18, 2024 18:35

github-actions Bot requested a review from ryanclark July 18, 2024 18:36

github-actions Bot added size/lg ui labels Jul 18, 2024

kimlisa reviewed Jul 19, 2024

View reviewed changes

avatus force-pushed the avatus/joinforms branch from dc1cb8b to 3aa99c9 Compare July 22, 2024 23:27

avatus requested a review from kimlisa July 22, 2024 23:28

ryanclark reviewed Jul 23, 2024

View reviewed changes

avatus requested a review from ryanclark July 23, 2024 16:08

avatus force-pushed the avatus/joinforms branch from 7a4ad95 to efc405f Compare July 24, 2024 16:15

ryanclark reviewed Jul 25, 2024

View reviewed changes

avatus requested a review from ryanclark July 25, 2024 16:21

kimlisa reviewed Jul 26, 2024

View reviewed changes

avatus requested a review from kimlisa July 26, 2024 12:11

ryanclark reviewed Jul 26, 2024

View reviewed changes

avatus requested a review from ryanclark July 26, 2024 16:22

ryanclark approved these changes Jul 26, 2024

View reviewed changes

avatus force-pushed the avatus/joinforms branch from f849632 to b118038 Compare July 26, 2024 17:25

avatus mentioned this pull request Jul 26, 2024

[16] Join Tokens UI #44701

Merged

kimlisa approved these changes Jul 26, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from rudream July 26, 2024 18:30

Add Allow and GCP fields to ui join token

21fe53a

avatus force-pushed the avatus/joinforms branch from 332f29e to 21fe53a Compare July 26, 2024 18:31

avatus enabled auto-merge July 26, 2024 18:33

avatus added this pull request to the merge queue Jul 26, 2024

Merged via the queue into master with commit 8e9cfc6 Jul 26, 2024

avatus deleted the avatus/joinforms branch July 26, 2024 19:05

This was referenced Jul 30, 2024

Machine ID: Add GitHub "Create Join Token" form #44794

Closed

Machine ID: Add GitLab "Create Join Token" form #44795

Closed

	rule={requiredField('At least 1 projectID required')}
	rule={requiredField('At least 1 project ID required')}

	h.PUT("/webapi/token/yaml", h.WithAuth(h.editTokenYAML))
	h.PUT("/webapi/token/yaml", h.WithAuth(h.updateTokenYAML))

	return nil, trace.BadParameter("renaming tokens is not supported.")
	return nil, trace.BadParameter("renaming tokens is not supported")

Conversation

avatus commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimlisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kimlisa commented Jul 19, 2024

Uh oh!

avatus commented Jul 19, 2024

Uh oh!

avatus commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus commented Jul 25, 2024

avatus commented Jul 18, 2024 •

edited

Loading