From f8e086a00b6ee0d88986a4d99947bcc50e8dd273 Mon Sep 17 00:00:00 2001 From: Bryan Frimin Date: Mon, 23 Mar 2026 08:54:55 +0100 Subject: [PATCH] Address PR review feedback for OIDC sign-in pages - Remove inline password form from SignInPage (use PasswordSignInPage) - Extract Divider and OIDCButtons to _components folder - Move OIDC providers into page queries instead of lazy-loaded queries - Create useSafeContinueUrl hook for trust app using getPathPrefix - Use safeContinueUrl.toString() for continue URL parameter - Fix wg.Go style in IAM service Run method Signed-off-by: Bryan Frimin --- .../src/pages/iam/auth/sign-in/SignInPage.tsx | 184 ++---------------- .../iam/auth/sign-in/_components/Divider.tsx | 10 + .../auth/sign-in/_components/OIDCButtons.tsx | 51 +++++ apps/trust/src/hooks/useSafeContinueUrl.ts | 36 ++++ apps/trust/src/pages/auth/ConnectPage.tsx | 62 ++---- pkg/iam/service.go | 48 +++-- 6 files changed, 165 insertions(+), 226 deletions(-) create mode 100644 apps/console/src/pages/iam/auth/sign-in/_components/Divider.tsx create mode 100644 apps/console/src/pages/iam/auth/sign-in/_components/OIDCButtons.tsx create mode 100644 apps/trust/src/hooks/useSafeContinueUrl.ts diff --git a/apps/console/src/pages/iam/auth/sign-in/SignInPage.tsx b/apps/console/src/pages/iam/auth/sign-in/SignInPage.tsx index 010e98d64..70e939ac3 100644 --- a/apps/console/src/pages/iam/auth/sign-in/SignInPage.tsx +++ b/apps/console/src/pages/iam/auth/sign-in/SignInPage.tsx @@ -1,34 +1,15 @@ -import { formatError, type GraphQLError } from "@probo/helpers"; import { useTranslate } from "@probo/i18n"; -import { Button, Field, Google, Microsoft, useToast } from "@probo/ui"; -import { type ComponentProps, type FormEventHandler, Suspense } from "react"; -import { useLazyLoadQuery, useMutation } from "react-relay"; -import { Link, matchPath, useLocation } from "react-router"; +import { Button } from "@probo/ui"; +import { useLazyLoadQuery } from "react-relay"; +import { Link, useLocation } from "react-router"; import { graphql } from "relay-runtime"; -import type { SignInPageMutation } from "#/__generated__/iam/SignInPageMutation.graphql"; import type { SignInPageQuery } from "#/__generated__/iam/SignInPageQuery.graphql"; import { useSafeContinueUrl } from "#/hooks/useSafeContinueUrl"; +import { Divider } from "./_components/Divider"; +import { OIDCButtons } from "./_components/OIDCButtons"; -const providerIcons: Record< - string, - (props: ComponentProps<"svg">) => React.ReactNode -> = { - google: Google, - microsoft: Microsoft, -}; - -const signInMutation = graphql` - mutation SignInPageMutation($input: SignInInput!) { - signIn(input: $input) { - session { - id - } - } - } -`; - -const oidcProvidersQuery = graphql` +const signInPageQuery = graphql` query SignInPageQuery { oidcProviders { name @@ -37,114 +18,12 @@ const oidcProvidersQuery = graphql` } `; -function Divider({ children }: { children: React.ReactNode }) { - return ( -
-
- - {children} - -
- ); -} - -function OIDCButtons() { - const { __ } = useTranslate(); - const safeContinueUrl = useSafeContinueUrl(); - - const data = useLazyLoadQuery(oidcProvidersQuery, {}); - - if (data.oidcProviders.length === 0) { - return null; - } - - return ( - <> - {data.oidcProviders.map((provider) => { - const Icon = providerIcons[provider.name]; - return ( - - ); - })} - - ); -} - export default function SignInPage() { const { __ } = useTranslate(); - const { toast } = useToast(); const location = useLocation(); const safeContinueUrl = useSafeContinueUrl(); - const [signIn, isSigningIn] - = useMutation(signInMutation); - - const handleSubmit: FormEventHandler = (e) => { - e.preventDefault(); - const formData = new FormData(e.currentTarget); - const email = (formData.get("email") as string) ?? ""; - const password = (formData.get("password") as string) ?? ""; - - if (!email || !password) return; - - const match = matchPath( - { - path: "/organizations/:organizationId", - caseSensitive: false, - end: false, - }, - safeContinueUrl.pathname, - ); - - signIn({ - variables: { - input: { - email, - password, - organizationId: match?.params.organizationId ?? null, - }, - }, - onCompleted: (_, error) => { - if (error) { - toast({ - title: __("Error"), - description: formatError( - __("Failed to sign in"), - error as GraphQLError, - ), - variant: "error", - }); - return; - } - - window.location.href = safeContinueUrl.href; - }, - onError: (e) => { - toast({ - title: __("Error"), - description: e.message, - variant: "error", - }); - }, - }); - }; + const data = useLazyLoadQuery(signInPageQuery, {}); return (
@@ -152,46 +31,23 @@ export default function SignInPage() { {__("Sign in to your account")} -
- + -
-
- - - {__("Forgot your password?")} - -
- -
+ {data.oidcProviders.length > 0 && ( + {__("Or")} + )} - - - -
- {__("Or")} - - - - + ); + })} + + ); +} diff --git a/apps/trust/src/hooks/useSafeContinueUrl.ts b/apps/trust/src/hooks/useSafeContinueUrl.ts new file mode 100644 index 000000000..34ce2e958 --- /dev/null +++ b/apps/trust/src/hooks/useSafeContinueUrl.ts @@ -0,0 +1,36 @@ +import { useMemo } from "react"; +import { useSearchParams } from "react-router"; + +import { getPathPrefix } from "#/utils/pathPrefix"; + +export function useSafeContinueUrl(): URL { + const [searchParams] = useSearchParams(); + + const continueUrlParam = searchParams.get("continue"); + const prefix = getPathPrefix(); + const fallback = window.location.origin + (prefix || "/"); + + const safeContinueUrl = useMemo(() => { + if (continueUrlParam) { + let continueUrl: URL; + try { + continueUrl = new URL(continueUrlParam, window.location.origin); + } catch { + return new URL(fallback, window.location.origin); + } + if ( + continueUrl.origin === window.location.origin + && continueUrl.pathname.startsWith(`${prefix}/`) + ) { + return new URL( + continueUrl.pathname + continueUrl.search, + window.location.origin, + ); + } + return new URL(fallback, window.location.origin); + } + return new URL(fallback, window.location.origin); + }, [continueUrlParam, fallback, prefix]); + + return safeContinueUrl; +} diff --git a/apps/trust/src/pages/auth/ConnectPage.tsx b/apps/trust/src/pages/auth/ConnectPage.tsx index b003dbae5..ccd785329 100644 --- a/apps/trust/src/pages/auth/ConnectPage.tsx +++ b/apps/trust/src/pages/auth/ConnectPage.tsx @@ -2,22 +2,20 @@ import type { GraphQLError } from "@probo/helpers"; import { usePageTitle } from "@probo/hooks"; import { useTranslate } from "@probo/i18n"; import { Button, Field, Google, Microsoft, useToast } from "@probo/ui"; -import { type ComponentProps, Suspense, useEffect, useRef, useState } from "react"; +import { type ComponentProps, useEffect, useRef, useState } from "react"; import { type PreloadedQuery, - useLazyLoadQuery, useMutation, usePreloadedQuery, } from "react-relay"; -import { useSearchParams } from "react-router"; import { graphql } from "relay-runtime"; import { z } from "zod"; import { useFormWithSchema } from "#/hooks/useFormWithSchema"; +import { useSafeContinueUrl } from "#/hooks/useSafeContinueUrl"; import { getPathPrefix } from "#/utils/pathPrefix"; import type { ConnectPageMutation, SendMagicLinkInput } from "./__generated__/ConnectPageMutation.graphql"; -import type { ConnectPageOIDCQuery } from "./__generated__/ConnectPageOIDCQuery.graphql"; import type { ConnectPageQuery } from "./__generated__/ConnectPageQuery.graphql"; export const connectPageQuery = graphql` @@ -27,11 +25,6 @@ export const connectPageQuery = graphql` name } } - } -`; - -const oidcProvidersQuery = graphql` - query ConnectPageOIDCQuery { oidcProviders { name loginURL @@ -74,20 +67,22 @@ function Divider({ children }: { children: React.ReactNode }) { ); } -function OIDCButtons({ safeContinueUrl }: { safeContinueUrl: string }) { +function OIDCButtons({ + providers, + safeContinueUrl, +}: { + providers: ReadonlyArray<{ readonly name: string; readonly loginURL: string }>; + safeContinueUrl: URL; +}) { const { __ } = useTranslate(); - const data = useLazyLoadQuery(oidcProvidersQuery, {}); - - if (data.oidcProviders.length === 0) { + if (providers.length === 0) { return null; } - const continueUrl = new URL(safeContinueUrl); - return ( <> - {data.oidcProviders.map((provider) => { + {providers.map((provider) => { const Icon = providerIcons[provider.name]; return (
- - - +
void handleSubmit(e)} className="space-y-6"> diff --git a/pkg/iam/service.go b/pkg/iam/service.go index d997d5f48..c3cea7061 100644 --- a/pkg/iam/service.go +++ b/pkg/iam/service.go @@ -180,32 +180,40 @@ func (s *Service) Run(ctx context.Context) error { defer cancel(context.Canceled) samlCtx, stopSAML := context.WithCancel(context.WithoutCancel(ctx)) - wg.Go(func() { - if err := s.SAMLService.Run(samlCtx); err != nil { - cancel(fmt.Errorf("saml service crashed: %w", err)) - } - }) + wg.Go( + func() { + if err := s.SAMLService.Run(samlCtx); err != nil { + cancel(fmt.Errorf("saml service crashed: %w", err)) + } + }, + ) oidcCtx, stopOIDC := context.WithCancel(context.WithoutCancel(ctx)) - wg.Go(func() { - if err := s.OIDCService.Run(oidcCtx); err != nil { - cancel(fmt.Errorf("oidc service crashed: %w", err)) - } - }) + wg.Go( + func() { + if err := s.OIDCService.Run(oidcCtx); err != nil { + cancel(fmt.Errorf("oidc service crashed: %w", err)) + } + }, + ) domainVerifierCtx, stopDomainVerifier := context.WithCancel(context.WithoutCancel(ctx)) - wg.Go(func() { - if err := s.samlDomainVerifier.Run(domainVerifierCtx); err != nil { - cancel(fmt.Errorf("saml domain verifier crashed: %w", err)) - } - }) + wg.Go( + func() { + if err := s.samlDomainVerifier.Run(domainVerifierCtx); err != nil { + cancel(fmt.Errorf("saml domain verifier crashed: %w", err)) + } + }, + ) scimCtx, stopSCIM := context.WithCancel(context.WithoutCancel(ctx)) - wg.Go(func() { - if err := s.SCIMService.Run(scimCtx); err != nil { - cancel(fmt.Errorf("scim service crashed: %w", err)) - } - }) + wg.Go( + func() { + if err := s.SCIMService.Run(scimCtx); err != nil { + cancel(fmt.Errorf("scim service crashed: %w", err)) + } + }, + ) <-ctx.Done()