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(core): Enable to change the name of the cookie for Admin API #2482

Merged

Conversation

gdarchen
Copy link
Contributor

Issue

Fixes #2481

Description

In this PR, I added a new authOptions.adminCookieName optional parameter in the Vendure config.
It makes it possible to give a custom name to the token set for the Admin API than the one given to the Shop API.

For now, here is the cookie naming behavior:

  • If not setting the authOptions.cookieOptions.name parameter:
    • Both the Admin API and Shop API cookies names will be the default one: 'session'
  • If setting the authOptions.cookieOptions.name parameter to something like 'cookie-name':
    • Both the Admin API and Shop API cookies names will be the configured one: 'cookie-name'

There is currently no way to give a custom name to the Admin API cookies, different from the Shop API ones.

With this changes, we can therefore set different cookies names for both APIs.

How to test

  1. In the dev-config.ts, change the authOptions as follows to name Shop API cookies shop-api and shop-api.sign, and the Admin API cookies admin-api and admin-api.sig:
     authOptions: {
         disableAuth: false,
         tokenMethod: ['bearer', 'cookie'] as const,
         requireVerification: true,
         customPermissions: [],
         cookieOptions: {
             name: "shop-api",
             secret: 'abc',
         },
         adminCookieName: 'admin-api'
     },
  2. Start the dev-server
  3. Open the GraphQL playground on the /admin-api and authenticate
  4. You should see the following cookies
    image
  5. Open the GraphQL playground on the /shop-api and authenticate
  6. You should now see the following cookies
    image

michaelbromley and others added 30 commits October 11, 2023 17:05
Co-authored-by: Alessio Marano <alessio.marano@sardexpay.net>
@netlify
Copy link

netlify bot commented Oct 23, 2023

Deploy Preview for effervescent-donut-4977b2 ready!

Name Link
🔨 Latest commit 203fd90
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/653ba2ead7cbc6000850026f
😎 Deploy Preview https://deploy-preview-2482--effervescent-donut-4977b2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@michaelbromley
Copy link
Member

I have a deep-seated aversion to adding new config properties to the Vendure config object. So here's another suggestion:

what if we change the type of the existing cookieOptions.name property to:

{
  name: string | { shop: string; admin: string; }
}

Internally the implementation could be similar, but just check the type of this property.

@gdarchen
Copy link
Contributor Author

I have a deep-seated aversion to adding new config properties to the Vendure config object. So here's another suggestion:

what if we change the type of the existing cookieOptions.name property to:

{
  name: string | { shop: string; admin: string; }
}

Internally the implementation could be similar, but just check the type of this property.

Done in ead93f1 (#2482)

I tested locally using the GraphQL Playgrounds and it seems to work as expected 😊

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good. I just suggested a small refactor to avoid duplication in the testing package and make maintenance easier.

Also I think this should go on the minor branch because it's a new API being added.

packages/core/src/bootstrap.ts Outdated Show resolved Hide resolved
@gdarchen gdarchen changed the base branch from master to minor October 27, 2023 11:46
@michaelbromley michaelbromley merged commit ae91650 into vendure-ecommerce:minor Oct 27, 2023
9 checks passed
@michaelbromley
Copy link
Member

Thank you!

@gdarchen
Copy link
Contributor Author

Hello @michaelbromley (and happy new year, it's allowed since we are still in January 🎉!),

Do you have an estimated release date of the future vendure@2.2 including this change?
This is really awaited by our admins ☺️

@michaelbromley
Copy link
Member

Hi,
No fixed date, but I'd like to get it released in February. In the mean time you could consider using the @next release (currently 2.2.0-next.1) which already includes this feature.

@daniel-lxs
Copy link

Is this feature released? or is it still pending?

@michaelbromley
Copy link
Member

final release of v2.2 will be next week

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.

Name differently the cookies for the Admin API and the Shop API