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

feat(backend,clerk-js,shared): Introduce suffixed / un-suffixed cookies #3274

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dimkl
Copy link
Member

@dimkl dimkl commented Apr 26, 2024

Description

Introduce suffixed / un-suffixed cookies to support multiple apps in the same domain.

TODOs

  • add integration test
  • add tests

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@dimkl dimkl self-assigned this Apr 26, 2024
Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: e73b3a2

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

This PR includes changesets to release 13 packages
Name Type
@clerk/clerk-js Minor
@clerk/backend Minor
@clerk/shared Minor
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/elements Patch
@clerk/clerk-react Patch
@clerk/testing 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

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: ec9b127

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

This PR includes changesets to release 12 packages
Name Type
@clerk/clerk-js Minor
@clerk/backend Minor
@clerk/shared Minor
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/clerk-react Patch
@clerk/testing 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

packages/clerk-js/src/core/clerk.ts Outdated Show resolved Hide resolved
packages/clerk-js/src/core/clerk.ts Outdated Show resolved Hide resolved
@dimkl dimkl mentioned this pull request May 8, 2024
9 tasks
@dimkl dimkl force-pushed the CORE-2086/suffixed-cookies branch 2 times, most recently from 5f3cb3c to 1c032dd Compare May 10, 2024 09:37
@dimkl dimkl marked this pull request as draft May 10, 2024 14:37
packages/backend/src/tokens/authenticateContext.ts Outdated Show resolved Hide resolved
.addOperation(new ReadSuffixedCookieOperation(cookieName, this.publishableKey))
.addOperation(new ReadCookieOperation(cookieName));
// get first result
return pipeline.invoke(this.clerkRequest.cookies).find(Boolean);
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Instead of introducing this Pipeline primitive, can we create some sort of cookies abstraction that handles dealing with the suffix?

This isn't a pattern we have elsewhere and I'm finding myself having to jump to the Pipeline definition to understand what's happening here. If we want to keep this abstraction, some comments might help explaining how it works.

packages/shared/src/pipelines.ts Outdated Show resolved Hide resolved
invoke(data: D): T;
}

export class Pipeline<T, D> implements Operation<T[]> {
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to introduce these new functional primitives, can we make sure they're clearly documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are going to introduce them i will add JSDoc with examples for each primitive :)

Comment on lines 69 to 72
const cookiePipeline = new Pipeline<string, string>()
.addOperation(new SuffixCookieOperation(authenticateContext.publishableKey))
.addOperation(new UnSuffixCookieOperation(authenticateContext.publishableKey));

Copy link
Member

Choose a reason for hiding this comment

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

Why do we define the pipeline here instead of closer to where we call it? Is it used by something in between?

Comment on lines 118 to 123
cookiePipeline.invoke(x).forEach(xVariants => {
if (xVariants.startsWith(`${constants.Cookies.Session}=`)) {
sessionToken = x.split(';')[0].split('=')[1];
}
headers.append('Set-Cookie', xVariants);
});
Copy link
Member

Choose a reason for hiding this comment

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

What would be the alternative way to write this without pipelines?

Comment on lines 8 to 16
addOperation(operation: Operation<T>) {
this.operations.push(operation);
return this;
}

invoke(data: D): T[] {
return this.operations.map(operation => operation.invoke(data));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand the pattern used here but I do need to do some more reading. This, however, looks like we're simply chaining methods while keeping a reference to their return value. Is this simply it?

Copy link
Member Author

@dimkl dimkl May 14, 2024

Choose a reason for hiding this comment

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

The Pipeline pattern is a mix of 2 more commonly known patterns:

This, however, looks like we're simply chaining methods while keeping a reference to their return value.

(kind of) yes.

Article about Pipeline pattern: https://medium.com/@bonnotguillaume/software-architecture-the-pipeline-design-pattern-from-zero-to-hero-b5c43d8a4e60

Comment on lines 18 to 22
export interface IOOperation<T, D = unknown, O = unknown> {
get(): T;
set(data: D, options: O): void;
remove(): void;
}
Copy link
Member

Choose a reason for hiding this comment

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

(This looks like the missing interface I was talking about in #3362)

This feels a little too complicated to me because it looks like we're defining 4 interfaces (and some duplicates across this PR and #3362 ) to simply invoke 2 methods. However, I want to think more about it as I'm not familiar with the pattern.

Could we achieve the same results by defining a method/helper with the following signature?

function writeCookie(handlers: CookieHandler[]) {
  handlers.forEach(h => h.write(...))
}

or similar? Would that be simpler?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR handles both generating multiple values based on a single input (case of backend SDK that uses a single cookie directive and produces the suffixed and un-suffixed variants) and performing multiple actions triggering a single pipeline method (eg write both suffixed and un-suffixed cookies using a single cookie key and value).
That's why there are multiple interfaces that look similar.
To achieve the same using method / helpers we should:

  • write 3 cookie handlers (eg session, clientUat, devBrowser) for ClerkJS
  • write 3 functions to generate the cookie key variants (eg session, clientUat, devBrowser) for backend SDK

For me this seems like the same amount of class or methods but different implementation. Using a pattern (even though it would require some time to learn it) would make the handling of all those cases consistent and expected instead of trying to read and verify what it's separate handler or function does.
If you think that it's more complex than it should, let's drop the pattern usage and simplify things. @nikosdouvlis Feel free to provide any suggestion on how we could make things simpler?

@dimkl
Copy link
Member Author

dimkl commented May 15, 2024

Would you consider this example of implementing the same pattern more readable and preferrable?

//
// @clerk/shared
//
type Operation<T> = (...args: any[]) => T;
const pipeline = <T>(operations: Operation<T>[], ...args: any[]) =>
  operations.map((o) => o(...args)).find(Boolean);

//
// @clerk/backend
//
const getCookieSuffixFromPk = (publishableKey: string): string => {
  return publishableKey.split("_").pop() || "";
};

const unSuffixCookie = (publishableKey: string) => {
  return (cookieDirective: string) => {
    //...
  };
};

const suffixCookie = (publishableKey: string) => {
  return (cookieDirective: string) => {
    //...
  };
};

// Example of executing the pipeline

const cookieDirectives = ["__session=aloha"];
cookieDirectives.map((cookieDirective) =>
  pipeline(
    [suffixCookie("pk_test_"), unSuffixCookie("pk_test_")],
    cookieDirective
  )
);

//
// @clerk/clerk-js
//

import { createCookieHandler } from "@clerk/shared/cookie";

const getCookie = () => {
  return (cookieName: string) => {
    //...
  };
};

const getSuffixedCookie = (publishableKey: string) => {
  return (cookieName: string) => {
    //...
  };
};

// Operations

const setCookie = () => {
  return (cookieName: string, value: string) => {
    //...
  };
};

const setSuffixedCookie = (publishableKey: string) => {
  return (cookieName: string, value: string) => {
    //...
  };
};

const removeCookie = () => {
  return (cookieName: string, value: string) => {
    //...
  };
};

const removeSuffixedCookie = (publishableKey: string) => {
  return (cookieName: string, value: string) => {
    //...
  };
};

// pipelines

const getPipeline = (cookieName: string) =>
  pipeline([getCookie(), getSuffixedCookie("pk_test_")], cookieName);
const setPipeline = (cookieName: string, value: string) =>
  pipeline([setCookie(), setSuffixedCookie("pk_test_")], cookieName, value);
const removePipeline = (cookieName: string) =>
  pipeline([removeCookie(), removeSuffixedCookie("pk_test_")], cookieName);

// Example of executing the pipeline

getPipeline("__session");
setPipeline("__session", "value");
removePipeline("__session");

cc: @BRKalow , @nikosdouvlis

@dimkl dimkl force-pushed the CORE-2086/suffixed-cookies branch from 1c032dd to e0fd02a Compare May 17, 2024 14:13
@dimkl dimkl marked this pull request as ready for review May 17, 2024 14:14
@dimkl dimkl force-pushed the CORE-2086/suffixed-cookies branch from 06cf509 to 3837991 Compare May 17, 2024 17:36
@dimkl dimkl force-pushed the CORE-2086/suffixed-cookies branch 3 times, most recently from fc0cb32 to ffdf6ea Compare May 23, 2024 19:44
dimkl added 3 commits May 24, 2024 12:52
…cookies

This change is required to support setting both suffixed/un-suffixed
cookies using part of the publishableKey to support Multiple apps running
on the same eTLD+1 domain or localhost.
Setting both suffixed/un-suffixed cookies is used to support backwards compatibility.
The optionsAssertions module will include assertion function for options
used across our package
@dimkl dimkl force-pushed the CORE-2086/suffixed-cookies branch from ffdf6ea to e73b3a2 Compare May 24, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants