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.

Sign up for GitHub

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

Jump to bottom

feat(backend,clerk-sdk-node): Simplify the authenticateRequest signature #1329

Merged
merged 1 commit into from
Jun 19, 2023
Merged

feat(backend,clerk-sdk-node): Simplify the authenticateRequest signature #1329

merged 1 commit into from
Jun 19, 2023

Conversation

anagstef
Copy link
Member

@anagstef anagstef commented Jun 9, 2023

Type of change

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

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

This PR simplifies the signature of authenticateRequest for better DX on both @clerk/backend and @clerk/clerk-sdk-node packages:

  • One pair of legacy or new instance keys are required now and not all 4 of them
  • @clerk/backend now can handle the "Bearer " prefix in Authorization header for better DX
  • host parameter is now optional in @clerk/backend

Example of using authenticateRequest

Before:

import clerkClient from '@clerk/clerk-sdk-node';

// `Bearer asdgfjdkhgmsfngl`
const authHeader = request.headers.get('authorization') || '';
const authToken = authHeader.split(` `)[1];

const res = await clerkClient.authenticateRequest({
  headerToken: authToken,
  publishableKey: process.env.CLERK_PUB_KEY,
  secretKey: process.env.CLERK_SECRET_KEY,
  apiKey: ``,
  frontendApi: ``,
  host: ``,
});

...

After this PR:

import clerkClient from '@clerk/clerk-sdk-node';

const res = await clerkClient.authenticateRequest({
  headerToken: request.headers.get('authorization'),
  publishableKey: process.env.CLERK_PUB_KEY,
  secretKey: process.env.CLERK_SECRET_KEY
});

...

@anagstef anagstef requested a review from dimkl June 9, 2023 00:08
@anagstef anagstef requested a review from SokratisVidros as a code owner June 9, 2023 00:08
@anagstef anagstef self-assigned this Jun 9, 2023
@anagstef anagstef requested a review from nikosdouvlis as a code owner June 9, 2023 00:08
@changeset-bot
Copy link

changeset-bot bot commented Jun 9, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: c2f124f

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

This PR includes changesets to release 6 packages
Name Type
@clerk/clerk-sdk-node Patch
@clerk/backend Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch
@clerk/fastify Patch
@clerk/remix 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

jit-ci[bot]
jit-ci bot reviewed Jun 9, 2023
View reviewed changes
Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

@anagstef anagstef force-pushed the stefanos/js-455-improve-the-dx-of-the-signature-of-authenticaterequest-for branch 2 times, most recently from 6da8e5e to af9b738 Compare June 9, 2023 12:14
@nikosdouvlis
Copy link
Member

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @nikosdouvlis - the snapshot version command generated the following package versions:

Package Version
@clerk/backend 0.22.0-snap.af9b738
@clerk/chrome-extension 0.3.8-snap.af9b738
@clerk/clerk-js 4.45.1-snap.af9b738
eslint-config-custom 0.3.0
@clerk/clerk-expo 0.17.8-snap.af9b738
@clerk/fastify 0.4.6-snap.af9b738
gatsby-plugin-clerk 4.3.7-snap.af9b738
@clerk/localizations 1.17.1-snap.af9b738
@clerk/nextjs 4.20.0-snap.af9b738
@clerk/clerk-react 4.19.0-snap.af9b738
@clerk/remix 2.6.6-snap.af9b738
@clerk/clerk-sdk-node 4.10.4-snap.af9b738
@clerk/shared 0.18.0-snap.af9b738
@clerk/themes 1.7.5
@clerk/types 3.41.1-snap.af9b738

Tip: use the snippet copy button below to quickly install the required packages.

# @clerk/backend
npm i @clerk/backend@0.22.0-snap.af9b738
# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.8-snap.af9b738
# @clerk/clerk-js
npm i @clerk/clerk-js@4.45.1-snap.af9b738
# eslint-config-custom
npm i eslint-config-custom@0.3.0
# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.17.8-snap.af9b738
# @clerk/fastify
npm i @clerk/fastify@0.4.6-snap.af9b738
# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.3.7-snap.af9b738
# @clerk/localizations
npm i @clerk/localizations@1.17.1-snap.af9b738
# @clerk/nextjs
npm i @clerk/nextjs@4.20.0-snap.af9b738
# @clerk/clerk-react
npm i @clerk/clerk-react@4.19.0-snap.af9b738
# @clerk/remix
npm i @clerk/remix@2.6.6-snap.af9b738
# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.10.4-snap.af9b738
# @clerk/shared
npm i @clerk/shared@0.18.0-snap.af9b738
# @clerk/themes
npm i @clerk/themes@1.7.5
# @clerk/types
npm i @clerk/types@3.41.1-snap.af9b738

@anagstef anagstef mentioned this pull request Jun 13, 2023
Closed
22 tasks
dimkl
dimkl approved these changes Jun 13, 2023
View reviewed changes
packages/backend/src/util/request.ts Outdated Show resolved Hide resolved
nikosdouvlis
nikosdouvlis approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

Good job @anagstef :)

.changeset/pink-carpets-matter.md Outdated Show resolved Hide resolved
packages/backend/src/index.ts Show resolved Hide resolved
packages/backend/src/tokens/request.ts
Comment on lines +56 to +63
publishableKey: string;
/**
* @deprecated Use `publishableKey` instead.
*/
frontendApi: string;
Copy link
Member

Choose a reason for hiding this comment

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

Why can we have both?

Copy link
Member

Choose a reason for hiding this comment

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

You have wayy more context that me here so please ignore if it doesn't make sense, however, the usual way to write this would be:

type ExclusiveAOrB = {
 a: string; 
 b?: never; 
} | {
 a?: never;
 b: string;
}

(notice the ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why can we have both?

@nikosdouvlis Isn't this a breaking change (on the type-level) of anyone passing both?

packages/sdk-node/src/authenticateRequest.ts Show resolved Hide resolved
packages/sdk-node/src/authenticateRequest.ts Show resolved Hide resolved
packages/sdk-node/src/types.ts Show resolved Hide resolved
@anagstef anagstef force-pushed the stefanos/js-455-improve-the-dx-of-the-signature-of-authenticaterequest-for branch from af9b738 to 680f728 Compare June 19, 2023 11:46
@anagstef
feat(backend,clerk-sdk-node): Simplify the authenticateRequest signature
c2f124f 
- One pair of legacy or new instance keys are required  instead of all 4 of them in `authenticateRequest`
- `@clerk/backend` now can handle the `Bearer` prefix in Authorization header for better DX
- `host` parameter is now optional in `@clerk/backend`
@anagstef anagstef force-pushed the stefanos/js-455-improve-the-dx-of-the-signature-of-authenticaterequest-for branch from 680f728 to c2f124f Compare June 19, 2023 15:40
@nikosdouvlis nikosdouvlis merged commit de2347f into main Jun 19, 2023
@nikosdouvlis nikosdouvlis deleted the stefanos/js-455-improve-the-dx-of-the-signature-of-authenticaterequest-for branch June 19, 2023 16:48
@clerk-cookie clerk-cookie mentioned this pull request Jun 19, 2023
Merged
mikestopcontinues pushed a commit to mikestopcontinues/clerk-javascript that referenced this pull request Jun 28, 2023
@anagstef @mikestopcontinues
feat(backend,clerk-sdk-node): Simplify the signature of authenticateR…
7e934f6 
…equest (clerk#1329)

- One pair of legacy or new instance keys are required  instead of all 4 of them in `authenticateRequest`
- `@clerk/backend` now can handle the `Bearer` prefix in Authorization header for better DX
- `host` parameter is now optional in `@clerk/backend`
@clerk-cookie
Copy link
Collaborator

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@clerk clerk locked as resolved and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jit-ci jit-ci[bot] jit-ci[bot] left review comments

@panteliselef panteliselef panteliselef approved these changes

@dimkl dimkl dimkl approved these changes

@nikosdouvlis nikosdouvlis nikosdouvlis approved these changes

@SokratisVidros SokratisVidros SokratisVidros approved these changes

Assignees

@anagstef anagstef

Labels
backend sdk-node
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anagstef @nikosdouvlis @clerk-cookie @SokratisVidros @dimkl @panteliselef