Skip to content

Update GQL client and auth middleware to handle invalid tokens and invalidate session #2259

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

Merged
merged 2 commits into from
May 7, 2025

Conversation

jordanarldt
Copy link
Contributor

@jordanarldt jordanarldt commented Apr 22, 2025

What/Why?

  • Update withAuth middleware to redirect protected routes to /login page when user is unauthorized
  • Add customer access token validation on protected routes
  • Minor updates to end-user error messages on unauthorized GQL requests to stop returning raw GQL error and use our translated error message instead.
  • Add onError event to GraphQL client
  • Use the onError callback to invalidate user session when GQL returns an invalid or missing token error

Testing

Screen.Recording.2025-05-02.at.5.54.09.PM.mov

Migration

  1. Copy all changes from the /core/client directory and the /packages/client directory
  2. Copy logic from withAuth middleware to handle the ?invalidate-session query param
  3. Copy translation values
  4. Copy all changes from the /core/app/[locale]/(default)/account/ directory server actions
  5. Copy all changes from the /core/app/[locale]/(default)/checkout/route.ts file

Copy link

changeset-bot bot commented Apr 22, 2025

🦋 Changeset detected

Latest commit: 8e7a907

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@bigcommerce/catalyst-client Patch
@bigcommerce/catalyst-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Apr 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
catalyst-canary ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2025 3:14pm
4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
catalyst ⬜️ Ignored (Inspect) May 7, 2025 3:14pm
catalyst-au ⬜️ Ignored (Inspect) Visit Preview May 7, 2025 3:14pm
catalyst-soul ⬜️ Ignored (Inspect) Visit Preview May 7, 2025 3:14pm
catalyst-uk ⬜️ Ignored (Inspect) Visit Preview May 7, 2025 3:14pm

@jordanarldt jordanarldt marked this pull request as ready for review April 22, 2025 21:19
@jordanarldt jordanarldt requested a review from a team as a code owner April 22, 2025 21:19
export const withAuth: MiddlewareFactory = (next) => {
return async (request, event) => {
// @ts-expect-error: The `auth` function doesn't have the correct type to support it as a MiddlewareFactory.
const authWithCallback = auth(async (req) => {
const isProtectedRoute = protectedPathRegExp.test(req.nextUrl.pathname);
const isGetRequest = req.method === 'GET';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only redirect on GET requests to avoid breaking any server actions.

@jordanarldt jordanarldt changed the title CATALYST-1115: Add token check and /login redirect in auth middleware Add token check and /login redirect in auth middleware Apr 23, 2025
@jordanarldt jordanarldt changed the title Add token check and /login redirect in auth middleware Update GQL client and auth middleware to handle invalid tokens and invalidate session May 2, 2025
Comment on lines +3 to +4
// eslint-disable-next-line @typescript-eslint/no-restricted-imports
import { redirect } from 'next/navigation';
Copy link
Contributor Author

@jordanarldt jordanarldt May 5, 2025

Choose a reason for hiding this comment

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

Using next/navigation here for a few reasons:

  1. For some reason importing ~/i18n/routing breaks this file and the build starts failing. (Module not found error)
  2. We don't need locale info for the session invalidation redirect, as it's just redirecting to the same route with the ?invalidate-session query param.

@@ -74,6 +85,7 @@ class Client<FetcherRequestInit extends RequestInit = RequestInit> {
fetchOptions?: FetcherRequestInit;
channelId?: string;
errorPolicy?: 'none' | 'all' | 'ignore';
validateCustomerAccessToken?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgemoya Yep, it's optional so that we don't have to update every single client.fetch in Catalyst to get this behavior. It's optional, but defaults to true, so if we want to override the behavior somewhere, we can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I guess my question was more in the sense of why would we want to override it but having the optional override shouldn't hurt.

Comment on lines +28 to +31
onError?: (
error: BigCommerceGQLError,
queryType: 'query' | 'mutation' | 'subscription',
) => Promise<void> | void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an existing pattern or something we built for this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's built for this use-case. As @chanceaclark was saying, we need the auth failure handling to be part of the DAL, which will require having this onError handler built into the client. 👍

@jordanarldt jordanarldt added this pull request to the merge queue May 7, 2025
Merged via the queue into canary with commit 67715bf May 7, 2025
9 checks passed
@jordanarldt jordanarldt deleted the CATALYST-1115 branch May 7, 2025 15:22
Copy link
Contributor

github-actions bot commented May 7, 2025

⚡️🏠 Lighthouse report

Lighthouse ran against https://catalyst-canary-et08lsoyd-bigcommerce-platform.vercel.app

🖥️ Desktop

We ran Lighthouse against the changes on a desktop and produced this report. Here's the summary:

Category Score
🟢 Performance 98
🟢 Accessibility 92
🟠 Best practices 78
🟢 SEO 91

📱 Mobile

We ran Lighthouse against the changes on a mobile and produced this report. Here's the summary:

Category Score
🟠 Performance 88
🟢 Accessibility 92
🟠 Best practices 78
🟢 SEO 92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants