Skip to content

0.7.0 #82

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

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

0.7.0 #82

wants to merge 27 commits into from

Conversation

sourabpramanik
Copy link
Collaborator

@sourabpramanik sourabpramanik commented Mar 24, 2025

? `${payload.config.cookiePrefix!}-token`
: `__${this.appName}-${APP_COOKIE_SUFFIX}`

const secret = this.useAdmin ? payload.secret : this.secret

Choose a reason for hiding this comment

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

In which way do these secrets differ and why not stick with one of them? Is this a security issue?

Choose a reason for hiding this comment

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

Maybe I'm wrong and not understanding correctly, but the useAdmin config option seems like it's handling something that this plugin shouldn't be doing.

If you're authenticated with Payload, then Payload should be handling whether a user is an admin through access.admin option on the collection.

But I suppose that's not available if the plugin is used instead of Payload's auth?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useAdmin defaults the session to Payload, so that Payload APIs can use it and validate users as admin.

If you're authenticated with Payload, then Payload should be handling whether a user is an admin through access.admin option on the collection.

But I suppose that's not available if the plugin is used instead of Payload's auth?

the cookie name and the secret used to generate the token is enough for Payload to verify that the user is an admin and can access the dashboard.

Using another secret for non-admin tokens is just to make sure there is no overlap and security around the user privileges

Choose a reason for hiding this comment

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

Payload handles it quite differently, hence my confusion: The CMS treats all users the same, and access control determines if a user is allowed to access the admin panel. I'd advocate for copying that behavior, so the plugin conforms more closely to the standards set by the CMS.

Default access control for the admin panel is admin: ({ req: { user } }) => Boolean(user) (all logged in users can access the admin panel).

In my project, I have a role check: admin: ({ req: { user } }) => checkRole(["admin"], user) (only users with role "admin" can access the admin panel).

If I'm not mistaken, this will work with useAdmin: true, but it's kind of double, don't you think?

As long as my functionality will still work, I'm totally okay. But I do think this is a part of the plugin that is more complicated than it needs to be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well it's nothing like that, actually I am using useAdmin just to use payload defaults to authenticate users and set all that parameters based on which Payload can process the request by checking if the user session is valid and checks all the boxes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When it comes to roles you can still specify the roles in the profile parameter of the register and oauth signin(with signups allowed) functions, doing it this way will save the user with a certain role. Now the plugin exports a server function to get the current user profile which is also dynamic as you can pass field names that you want to query that can be a role too

* @default Cookie
*
*/
authenticationStrategy?: AuthenticationStrategy

Choose a reason for hiding this comment

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

This change basically means moving closer to Payload's auth, where both Cookie + JWT are active by default, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, somehow. I removed it because I have some other plans to support different strategies

Choose a reason for hiding this comment

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

Sounds great.

@engin-can
Copy link

Hey @sourabpramanik , do you happen to have an ETA for this release?

@Kotletka89
Copy link

Hey @sourabpramanik , do you happen to have an ETA for this release?

I'm also very interested in it 🥹

@Kotletka89
Copy link

image

@sourabpramanik
Copy link
Collaborator Author

You can expect it next week any time. I have been away this week because of my job that's why I had to push

@@ -0,0 +1,9 @@
'use client'

import { appClient } from 'payload-auth-plugin/client'

Choose a reason for hiding this comment

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

Isn't appClient deprecated? I think this file still has to be adapted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is

@vzheng56
Copy link

vzheng56 commented Apr 7, 2025

Hi @sourabpramanik, I noticed that the merged auth plugin (version 0.7) is still in progress. I understand from the comments that you mentioned it would be released "next week" (which was around 1-2 weeks ago). I'm really looking forward to using these new features in my project, especially the unified auth plugin approach. Would it be possible to get an update on the expected release timeline? Thanks for all your work on this - it looks like a great improvement to the plugin!

@Rot4tion
Copy link

Rot4tion commented May 2, 2025

bump

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.

8 participants