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 <bryan@getprobo.com>

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 { Button } from "@probo/ui";
-import { type ComponentProps, type FormEventHandler, Suspense } from "react";
+import { useLazyLoadQuery } from "react-relay";
-import { useLazyLoadQuery, useMutation } from "react-relay";
+import { Link, useLocation } from "react-router";
-import { Link, matchPath, 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<
+const signInPageQuery = graphql`
-  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`
   query SignInPageQuery {
     oidcProviders {
       name
@@ -37,114 +18,12 @@ const oidcProvidersQuery = graphql`
   }
 `;
 
-function Divider({ children }: { children: React.ReactNode }) {
-  return (
-    <div className="relative my-6 w-full">
-      <div className="border-t border-border-mid" />
-      <span className="px-4 text-xs uppercase text-txt-secondary bg-level-0 absolute top-0 left-1/2 -translate-1/2">
-        {children}
-      </span>
-    </div>
-  );
-}
-
-function OIDCButtons() {
-  const { __ } = useTranslate();
-  const safeContinueUrl = useSafeContinueUrl();
-
-  const data = useLazyLoadQuery<SignInPageQuery>(oidcProvidersQuery, {});
-
-  if (data.oidcProviders.length === 0) {
-    return null;
-  }
-
-  return (
-    <>
-      {data.oidcProviders.map((provider) => {
-        const Icon = providerIcons[provider.name];
-        return (
-          <Button
-            key={provider.name}
-            variant="secondary"
-            className="w-full h-10"
-            onClick={() => {
-              window.location.href
-                = provider.loginURL
-                  + "?continue="
-                  + encodeURIComponent(
-                    safeContinueUrl.pathname + safeContinueUrl.search,
-                  );
-            }}
-          >
-            <span className="flex items-center gap-2">
-              {Icon && <Icon width={18} height={18} />}
-              {__(`Sign in with ${provider.name.charAt(0).toUpperCase() + provider.name.slice(1)}`)}
-            </span>
-          </Button>
-        );
-      })}
-    </>
-  );
-}
-
 export default function SignInPage() {
   const { __ } = useTranslate();
-  const { toast } = useToast();
   const location = useLocation();
   const safeContinueUrl = useSafeContinueUrl();
 
-  const [signIn, isSigningIn]
+  const data = useLazyLoadQuery<SignInPageQuery>(signInPageQuery, {});
-    = useMutation<SignInPageMutation>(signInMutation);
-
-  const handleSubmit: FormEventHandler<HTMLFormElement> = (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",
-        });
-      },
-    });
-  };
 
   return (
     <div className="w-full max-w-sm mx-auto pt-8">
@@ -152,46 +31,23 @@ export default function SignInPage() {
         {__("Sign in to your account")}
       </h1>
 
-      <form className="mt-6 space-y-4" onSubmit={handleSubmit}>
-        <Field
-          required
-          name="email"
-          type="email"
-          label={__("Email")}
-          autoFocus
-        />
-
-        <div>
-          <div className="flex items-center justify-between mb-1">
-            <label className="text-sm font-medium" htmlFor="password">
-              {__("Password")}
-            </label>
-            <Link
-              to="/auth/forgot-password"
-              className="text-sm text-txt-secondary hover:text-txt-primary"
-            >
-              {__("Forgot your password?")}
-            </Link>
-          </div>
-          <Field
-            required
-            name="password"
-            id="password"
-            type="password"
-          />
-        </div>
-
-        <Button className="w-full h-10" disabled={isSigningIn}>
-          {isSigningIn ? __("Signing in...") : __("Sign in")}
-        </Button>
-      </form>
-
       <div className="mt-6 space-y-4">
-        <Divider>{__("Or")}</Divider>
+        <OIDCButtons
+          providers={data.oidcProviders}
+          safeContinueUrl={safeContinueUrl}
+        />
 
-        <Suspense fallback={null}>
+        {data.oidcProviders.length > 0 && (
-          <OIDCButtons />
+          <Divider>{__("Or")}</Divider>
-        </Suspense>
+        )}
+
+        <Button
+          variant="secondary"
+          className="w-full h-10"
+          to={{ pathname: "/auth/password-login", search: location.search }}
+        >
+          {__("Sign in with Email")}
+        </Button>
 
         <Button
           variant="secondary"

apps/console/src/pages/iam/auth/sign-in/_components/Divider.tsx

@@ -0,0 +1,10 @@
+export function Divider({ children }: { children: React.ReactNode }) {
+  return (
+    <div className="relative my-6 w-full">
+      <div className="border-t border-border-mid" />
+      <span className="px-4 text-xs uppercase text-txt-secondary bg-level-0 absolute top-0 left-1/2 -translate-1/2">
+        {children}
+      </span>
+    </div>
+  );
+}

apps/console/src/pages/iam/auth/sign-in/_components/OIDCButtons.tsx

@@ -0,0 +1,51 @@
+import { useTranslate } from "@probo/i18n";
+import { Button, Google, Microsoft } from "@probo/ui";
+import type { ComponentProps } from "react";
+
+const providerIcons: Record<
+  string,
+  (props: ComponentProps<"svg">) => React.ReactNode
+> = {
+  google: Google,
+  microsoft: Microsoft,
+};
+
+export function OIDCButtons({
+  providers,
+  safeContinueUrl,
+}: {
+  providers: ReadonlyArray<{ readonly name: string; readonly loginURL: string }>;
+  safeContinueUrl: URL;
+}) {
+  const { __ } = useTranslate();
+
+  if (providers.length === 0) {
+    return null;
+  }
+
+  return (
+    <>
+      {providers.map((provider) => {
+        const Icon = providerIcons[provider.name];
+        return (
+          <Button
+            key={provider.name}
+            variant="secondary"
+            className="w-full h-10"
+            onClick={() => {
+              window.location.href
+                = provider.loginURL
+                  + "?continue="
+                  + encodeURIComponent(safeContinueUrl.toString());
+            }}
+          >
+            <span className="flex items-center gap-2">
+              {Icon && <Icon width={18} height={18} />}
+              {__(`Sign in with ${provider.name.charAt(0).toUpperCase() + provider.name.slice(1)}`)}
+            </span>
+          </Button>
+        );
+      })}
+    </>
+  );
+}

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;
+}

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<ConnectPageOIDCQuery>(oidcProvidersQuery, {});
+  if (providers.length === 0) {
-
-  if (data.oidcProviders.length === 0) {
     return null;
   }
 
-  const continueUrl = new URL(safeContinueUrl);
-
   return (
     <>
-      {data.oidcProviders.map((provider) => {
+      {providers.map((provider) => {
         const Icon = providerIcons[provider.name];
         return (
           <Button
@@ -98,9 +93,7 @@ function OIDCButtons({ safeContinueUrl }: { safeContinueUrl: string }) {
               window.location.href
                 = provider.loginURL
                   + "?continue="
-                  + encodeURIComponent(
+                  + encodeURIComponent(safeContinueUrl.toString());
-                    continueUrl.pathname + continueUrl.search,
-                  );
             }}
           >
             <span className="flex items-center gap-2">
@@ -125,29 +118,13 @@ export function ConnectPage(props: {
   const [magicLinkSent, setMagicLinkSent] = useState<boolean>(false);
   const interval = useRef<NodeJS.Timeout>(undefined);
   const [timer, setTimer] = useState<number>(timerDurationSeconds);
-  const [searchParams] = useSearchParams();
+  const safeContinueUrl = useSafeContinueUrl();
 
   const {
     currentTrustCenter: { organization },
+    oidcProviders,
   } = usePreloadedQuery<ConnectPageQuery>(connectPageQuery, queryRef);
 
-  const continueUrlParam = searchParams.get("continue");
-  let safeContinueUrl: string;
-  if (continueUrlParam) {
-    try {
-      const continueUrl = new URL(continueUrlParam, window.location.origin);
-      if (continueUrl.origin === window.location.origin && continueUrl.pathname.startsWith(`${getPathPrefix()}/`)) {
-        safeContinueUrl = window.location.origin + continueUrl.pathname + continueUrl.search;
-      } else {
-        safeContinueUrl = window.location.origin + getPathPrefix();
-      }
-    } catch {
-      safeContinueUrl = window.location.origin + getPathPrefix();
-    }
-  } else {
-    safeContinueUrl = window.location.origin + getPathPrefix();
-  }
-
   useEffect(() => {
     if (!magicLinkSent && interval.current) {
       clearInterval(interval.current);
@@ -184,13 +161,13 @@ export function ConnectPage(props: {
   const handleSubmit = handleSubmitWrapper(({ email }: FormData) => {
     const input: SendMagicLinkInput = { email };
     if (safeContinueUrl) {
-      input.continue = safeContinueUrl;
+      input.continue = safeContinueUrl.toString();
     }
     sendMagicLink({
       variables: {
         input: {
           email,
-          continue: safeContinueUrl,
+          continue: safeContinueUrl.toString(),
         },
       },
       onCompleted: (_, errors: GraphQLError[] | null) => {
@@ -241,9 +218,10 @@ export function ConnectPage(props: {
       </div>
 
       <div className="space-y-4">
-        <Suspense fallback={null}>
+        <OIDCButtons
-          <OIDCButtons safeContinueUrl={safeContinueUrl} />
+          providers={oidcProviders}
-        </Suspense>
+          safeContinueUrl={safeContinueUrl}
+        />
       </div>
 
       <form onSubmit={e => void handleSubmit(e)} className="space-y-6">

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() {
+	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() {
+	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() {
+	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() {
+	wg.Go(
+		func() {
 			if err := s.SCIMService.Run(scimCtx); err != nil {
 				cancel(fmt.Errorf("scim service crashed: %w", err))
 			}
-	})
+		},
+	)
 
 	<-ctx.Done()