Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Web] Separate Auth Connector UI into dedicated pages #50749

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rudream
Copy link
Contributor

@rudream rudream commented Jan 5, 2025

Purpose

Part of https://github.com/gravitational/teleport.e/issues/4989

e counterpart: https://github.com/gravitational/teleport.e/pull/5789

This PR adds separate dedicated pages for auth connectors to the UI, instead of the current flow which uses dialogs.

Figma designs: https://www.figma.com/design/v6GunK50D2VC7w7I2FBDNf/Access-(Management)?node-id=6862-43134&m=dev

Demo

Adding a new connector

image image

Editing an existing connector

image

@rudream rudream added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Jan 5, 2025
@rudream rudream force-pushed the yassine/auth-connector-ui branch from 08614c8 to ee4039e Compare January 6, 2025 01:16
Comment on lines +63 to +65
<Flex mr={4} alignItems="baseline">
<H1>{title}</H1>
</Flex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for baseline here? there arent any other elements, could this just be a box?

Comment on lines +63 to +65
<Flex mr={4} alignItems="baseline">
<H1>{title}</H1>
</Flex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for baseline here? there arent any other elements, could this just be a box?

Comment on lines +72 to +74
<Box textAlign="center" m={10}>
<Indicator />
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this margin is used to center, you could maybe use Flex with some justify and align instead of adding a bunch of margin. doesnt really matter tho

Comment on lines +84 to +86
{saveAttempt.status === 'failed' && (
<Alert>{saveAttempt.statusText}</Alert>
)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is the alert, but its not full width
Screenshot 2025-01-06 at 9 15 29 AM

Comment on lines +89 to +94
<TextEditor
bg="levels.deep"
readOnly={false}
data={[{ content, type: 'yaml' }]}
onChange={setContent}
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this full width editor instead of being in a dialog is soooooo nice. feels much more "solid"

Comment on lines +114 to +124
<Link
color="text.main"
// This URL is the OSS documentation for auth connectors
href="https://goteleport.com/docs/setup/admin/github-sso/"
target="_blank"
>
view our documentation
</Link>{' '}
on how to configure a GitHub connector.
</P>
</DesktopDescription>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should update to whatever type of connector we are trying to add. currently this says github even when on OIDC and SAML

Comment on lines +207 to +212
resource, err := ui.NewResourceItem(connector)
if err != nil {
return nil, trace.Wrap(err)
}

return resource, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
resource, err := ui.NewResourceItem(connector)
if err != nil {
return nil, trace.Wrap(err)
}
return resource, nil
return ui.NewResourceItem(connector)

Please{' '}
<Link
color="text.main"
// This URL is the OSS documentation for auth connectors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reads redundant

Suggested change
// This URL is the OSS documentation for auth connectors

@@ -0,0 +1,122 @@
/**
* Teleport
* Copyright (C) 2023 Gravitational, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright (C) 2023 Gravitational, Inc.
* Copyright (C) 2024 Gravitational, Inc.

const [content, setContent] = useState(templates['github']);
const [initialContent, setInitialContent] = useState(templates['github']);

const fetchAttempt = useAttempt(isNew ? 'success' : 'processing');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we now favoring useAsync from shared/hooks/useAsync over useAttemptNext?

useAsync returns attempt and run which you can then call below in useEffect.

Same comment for saveAttempt below and in other files.

Comment on lines +207 to +213
// sso routes
/**
* ssoConnectorCreate is the dedicated page for creating a new auth connector.
*/
ssoConnectorCreate: '/web/sso/new/:connectorType(github|oidc|saml)',
ssoConnectorEdit:
'/web/sso/edit/:connectorType(github|oidc|saml)/:connectorName?',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: group these together in an object, makes it easier to find/update them later.

ssoConnector : {
  Create: '/web/sso/new/:connectorType(github|oidc|saml)',
  Edit: '/web/sso/edit/:connectorType(github|oidc|saml)/:connectorName?'
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants