Org SSO Page updates #16868

selfcontained · 2023-03-15T18:45:17Z

Description

Makes various updates to the Org Settings => SSO page.

Adds Delete support for OIDC clients
Adds Edit support to the UI for OIDC clients (api needs implementing still)
Updates page to use react-query
Updates page to take advantage of existing components similar to Git Auth.

Related Issue(s)

Relates to #15961

How to test

Preview Env: https://bmh-org-se0fee9b85fb.preview.gitpod-dev.com/workspaces

Create an Org, visit the SSO page
Create a client.
Try to edit it. UI should populate w/ client data, but saving should product an error as the API isn't implemented yet.
Try to delete the client

Release Notes

NONE

Documentation

Build Options:

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish Options

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer Options

with-ee-license
with-dedicated-emulation
with-ws-manager-mk2
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2023-03-15T18:45:25Z

started the job as gitpod-build-bmh-org-settings-sso-updates.1 because the annotations in the pull request description changed
(with .werft/ from main)

selfcontained · 2023-03-15T22:01:41Z

components/dashboard/src/teams/git-integrations/GitIntegrations.tsx

+
+    return (
+        <div>
+            <Heading2>Git Auth configurations</Heading2>


Adding a heading/subheading to the content of the git auth page so it's a bit more consistent w/ the SSO page layout. Open to suggestions for different copy.

selfcontained · 2023-03-15T22:10:59Z

components/dashboard/src/components/Modal.tsx

@@ -150,7 +150,7 @@ export const ModalFooter: FC<ModalFooterProps> = ({ error, warning, children })
    return (
        <div className="relative">
            {hasAlert && (
-                <div className="absolute -top-12 left-0 right-0" style={alertStyles}>
+                <div className="absolute bottom-12 left-0 right-0" style={alertStyles}>


Positioning off of the bottom allows it to grow taller if needed (still some styling issues for long error messages, but this should be a small improvement).

issue: Using the alert component here sounds perfect, however, using alerts or error feedback inside modals can be tricky, especially after adding a scrollable area inside the modal in #15084.

Yup, I think having the alert positioned absolutely somewhere so it's kind of pinned at the footer is one way to avoid some of those issues (but does introduce some other challenges). Currently I'm using a pattern where the alert is pinned right above the footer (part of the footer actually). I've added some extra padding to the bottom of the modal body section to account for it, but that's a temporary solution and not ideal. What I'd like to do:

Animate the alert so it slides up from the footer.

Find a more robust way to deal with the alert potentially blocking content in the modal body. Some ideas:

Dynamically adding padding to the bottom of the modal body based on the size of the alert when it's present so content can be scrolled to in the body.

Making the pinned modal alert dismissible (probably the easiest solution to implement).

selfcontained · 2023-03-15T22:13:18Z

components/dashboard/src/teams/GitIntegrationsPage.tsx

    return (
-        <OrgSettingsPageWrapper>
+        <OrgSettingsPage title="Git Auth" subtitle="Configure Git Auth for GitLab or Github.">


Wrapper component no longer needed, as the loading state is handled in <OrgSettingsPage/> now.

issue: I wanted to mention this in #16850, but the goal of the settings page was to keep the Organizations Settings in the header around for all pages under settings, and rely on the left sidebar for navigation and less duplication of labels. Cc @AlexTugarev

Example of labels repeating

Oh, that's my mistake then, I understood some of that differently. I can change this back if that's what we want in a followup pretty easily. It is kinda awkward coming up with 3 versions of the page title that are slightly different, so this would remove that 😄

selfcontained · 2023-03-15T22:16:09Z

components/public-api-server/pkg/apiv1/oidc.go

@@ -301,6 +301,9 @@ func dbOIDCClientConfigToAPI(config db.OIDCClientConfig, decryptor db.Decryptor)
 			AuthorizationEndpoint: decrypted.RedirectURL,
 			Scopes:                decrypted.Scopes,
 		},
+		OidcConfig: &v1.OIDCConfig{


Noticed oidcConfig.issuer wasn't returned in our responses for clients. This returns it, but would like to make sure it wasn't intentional that we weren't returning it.

It was probably overlooked, because it wasn't used anywhere. Thanks for adding it 🙏🏻

selfcontained · 2023-03-15T22:19:30Z

/hold for feedback

AlexTugarev · 2023-03-16T06:53:39Z

components/dashboard/src/teams/sso/OIDCClientConfigModal.tsx

+        }
+    }, [clientConfig?.id, clientId, clientSecret, isNew, isValid, issuer, onClose, org, upsertClientConfig]);
+
+    const errorMessage = upsertClientConfig.isError ? "There was a problem saving your configuration." : "";


It would be great to surface those errors.

Especially for the backend validation those errors are crucial to identify connectivity problems between Gitpod's backend and the IdP services. The client side validation is nice and a good improvement, but if the backend cannot connect to the services, the auth processes will break too late on receiving the callback from IdP.

For example if you enter some valid URL, which might point to non-existing host, we see the following error from backend. I agree it's a bit too long, we might need to tweak that (on backend) as well. While this is about non exiting host, same applies for hosts which are not reachable from the network the backend services are running in, so basically it's a whole class of connectivity issues.

Get "https://google234444444.com": dial tcp: lookup google234444444.com on 10.43.0.10:53: no such host

Yah, I don't have the modal-based error message component built in a way that supports a dynamic amount of content very well atm, but I agree, we should be able to include those details in the error message, just need to make it more robust so it can expand to fit content, but also not block the modal body content it slides over. I'll tackle that in a follow-up.

AlexTugarev

Looks great! A lot of improvements here, again 🙏🏻

The only point I have is to surface backend errors, see https://github.com/gitpod-io/gitpod/pull/16868/files#r1138198412.

This can also be done separately, so I leave it up to you to land this.

gtsiolis · 2023-03-16T12:40:38Z

Looking at this now! 👀

gtsiolis

UX LGTM!

Additionally approving to unblock merging and holding in case we'd like to address any of the UX issues in this PR. 🏓

/hold

gtsiolis · 2023-03-16T12:17:57Z

components/dashboard/src/teams/GitIntegrationsPage.tsx

    return (
-        <OrgSettingsPageWrapper>
+        <OrgSettingsPage title="Git Auth" subtitle="Configure Git Auth for GitLab or Github.">


issue: I wanted to mention this in #16850, but the goal of the settings page was to keep the Organizations Settings in the header around for all pages under settings, and rely on the left sidebar for navigation and less duplication of labels. Cc @AlexTugarev

Example of labels repeating

gtsiolis · 2023-03-16T12:19:33Z

components/dashboard/src/teams/GitIntegrationsPage.tsx

    return (
-        <OrgSettingsPageWrapper>
+        <OrgSettingsPage title="Git Auth" subtitle="Configure Git Auth for GitLab or Github.">
            <GitIntegrations />


thought: We can probably update the copy in the empty state for Git integrations, but we can make these in a separate PR.

gtsiolis · 2023-03-16T12:20:10Z

components/dashboard/src/teams/GitIntegrationsPage.tsx

    return (
-        <OrgSettingsPageWrapper>
+        <OrgSettingsPage title="Git Auth" subtitle="Configure Git Auth for GitLab or Github.">
            <GitIntegrations />


praise: This is out of the scope of the changes in this PR, but wanted to say thanks for improving the copy in #16850 from the previous iteration. 🌮 🌮

gtsiolis · 2023-03-16T12:34:46Z

components/dashboard/src/components/Modal.tsx

@@ -150,7 +150,7 @@ export const ModalFooter: FC<ModalFooterProps> = ({ error, warning, children })
    return (
        <div className="relative">
            {hasAlert && (
-                <div className="absolute -top-12 left-0 right-0" style={alertStyles}>
+                <div className="absolute bottom-12 left-0 right-0" style={alertStyles}>


issue: Using the alert component here sounds perfect, however, using alerts or error feedback inside modals can be tricky, especially after adding a scrollable area inside the modal in #15084.

gtsiolis · 2023-03-16T12:39:14Z

components/dashboard/src/components/Modal.tsx

@@ -150,7 +150,7 @@ export const ModalFooter: FC<ModalFooterProps> = ({ error, warning, children })
    return (
        <div className="relative">
            {hasAlert && (
-                <div className="absolute -top-12 left-0 right-0" style={alertStyles}>
+                <div className="absolute bottom-12 left-0 right-0" style={alertStyles}>


suggestion: Since we're having validation inline for text input, we could do the following in this or a follow PR:

Skip the alert here and rely on the SSO clients list to communicate an error or a failed attempt.

Migrate away from the modals to a full page layout as we do with the access tokens flow, which is more appropriate, scalable, and accessible.

Input validation Error feedback through list Error feedback in full page

Good points.

If we stuck with the modal, we'd need some version of an alert for any error messages returned from the api call during create/update.

I'd be on board with using full pages here. I debated going down that route, but felt like a bigger change I didn't want to introduce quite yet. It would also simplify some of the complexity in the git auth modal that deals with managing state after creation.

Either way, I think having a way to show simple error messages for a modal in a consistent way will helpful, but need to address some of the usability points we've discussed.

gtsiolis · 2023-03-16T12:42:06Z

components/dashboard/src/teams/sso/OIDCClients.tsx

+            {clientConfigs.length === 0 ? (
+                <EmptyMessage
+                    title="No OIDC providers"
+                    subtitle="Enable single sign-on for your organization using an external identity provider (IdP) service that supports the OpenID Connect (OIDC) standard, such as Google."


thought: I'm sure we can come up with a better and more readable copy here, but let's do that in a separate PR.

selfcontained · 2023-03-16T14:58:39Z

/werft run with-preview=true

👍 started the job as gitpod-build-bmh-org-settings-sso-updates.5
(with .werft/ from main)

selfcontained · 2023-03-16T20:58:12Z

/unhold

Breaking SSO page into components & react-query

b4188f7

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Mar 15, 2023

roboquat added the size/XL label Mar 15, 2023

selfcontained added 4 commits March 15, 2023 21:07

wrapper component not needed anymore

58c4f8a

position off of bottom instead

ccf926c

add a heading/subheading for consistency

06c2f5a

add clientside validation

85a7fa1

roboquat added size/XXL and removed size/XL labels Mar 15, 2023

adding oidcConfig.issuer to api response

f444527

selfcontained changed the title ~~Breaking SSO page into components & react-query~~ Org SSO Page updates Mar 15, 2023

updating test

3faf961

selfcontained commented Mar 15, 2023

View reviewed changes

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Mar 15, 2023

selfcontained commented Mar 15, 2023

View reviewed changes

selfcontained marked this pull request as ready for review March 15, 2023 22:16

selfcontained requested a review from a team March 15, 2023 22:16

selfcontained requested a review from gtsiolis as a code owner March 15, 2023 22:16

roboquat removed the do-not-merge/work-in-progress label Mar 15, 2023

github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 15, 2023

roboquat added the do-not-merge/hold label Mar 15, 2023

AlexTugarev reviewed Mar 16, 2023

View reviewed changes

AlexTugarev approved these changes Mar 16, 2023

View reviewed changes

gtsiolis approved these changes Mar 16, 2023

View reviewed changes

minor cleanup

b9372e2

roboquat removed the do-not-merge/hold label Mar 16, 2023

roboquat merged commit ff079b9 into main Mar 16, 2023

roboquat deleted the bmh/org-settings-sso-updates branch March 16, 2023 20:59

selfcontained mentioned this pull request Mar 17, 2023

Org Settings and Modal alert updates #16883

Merged

14 tasks

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Mar 20, 2023

AlexTugarev mentioned this pull request Mar 28, 2023

[SSO] Support edit and remove of OIDC clients #15961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Org SSO Page updates #16868

Org SSO Page updates #16868

selfcontained commented Mar 15, 2023 •

edited

Loading

werft-gitpod-dev-com bot commented Mar 15, 2023

selfcontained Mar 15, 2023 •

edited

Loading

selfcontained Mar 15, 2023

gtsiolis Mar 16, 2023 •

edited

Loading

selfcontained Mar 16, 2023

selfcontained Mar 15, 2023

gtsiolis Mar 16, 2023

selfcontained Mar 16, 2023

selfcontained Mar 15, 2023

AlexTugarev Mar 16, 2023

selfcontained commented Mar 15, 2023

AlexTugarev Mar 16, 2023

selfcontained Mar 16, 2023

AlexTugarev left a comment

gtsiolis commented Mar 16, 2023 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment

gtsiolis Mar 16, 2023

gtsiolis Mar 16, 2023

gtsiolis Mar 16, 2023

gtsiolis Mar 16, 2023 •

edited

Loading

gtsiolis Mar 16, 2023

selfcontained Mar 16, 2023

gtsiolis Mar 16, 2023

gtsiolis Mar 16, 2023

selfcontained commented Mar 16, 2023 •

edited by werft-gitpod-dev-com bot

Loading

selfcontained commented Mar 16, 2023

Org SSO Page updates #16868

Org SSO Page updates #16868

Conversation

selfcontained commented Mar 15, 2023 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Build Options:

Preview Environment Options:

werft-gitpod-dev-com bot commented Mar 15, 2023

selfcontained Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfcontained commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

gtsiolis commented Mar 16, 2023 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfcontained commented Mar 16, 2023 • edited by werft-gitpod-dev-com bot Loading

selfcontained commented Mar 16, 2023

selfcontained commented Mar 15, 2023 •

edited

Loading

selfcontained Mar 15, 2023 •

edited

Loading

gtsiolis Mar 16, 2023 •

edited

Loading

gtsiolis commented Mar 16, 2023 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis Mar 16, 2023 •

edited

Loading

selfcontained commented Mar 16, 2023 •

edited by werft-gitpod-dev-com bot

Loading