-
-
Notifications
You must be signed in to change notification settings - Fork 36
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: support passing data to middleware from actions #67
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
It seems to me that this is not useful for middleware at all. In middleware, you can get the role of the current user from the user context, or use an additional request. export const authAction = createSafeActionClient({
async middleware(parsedInput) {
const session = cookies().get('session')?.value
if (!session) {
throw new Error('Session not found!')
}
// In the real world, you would check if the session is valid by querying a database.
// We'll keep it very simple here.
const user = await getUserFromSession(session)
if (!user) {
throw new Error('Session is not valid!')
}
const request = ...
const pathname = request.pathname
if (['/admin']?.some((path) => pathname.startsWith(path)) && user.role !== 'admin') {
throw new Error('You do not have permission to execute this action!')
}
return { user }
}
}) Another question is how to send a request here. It's worth thinking about. When passing a request, there are a lot more possibilities here. In addition to adding a request, it will be useful to connect several middlewares. Then we could separate them: // chain middlewares
export type MiddlewareFactory = (middleware: Middleware) => Middleware
export default function middlewares(
functions: MiddlewareFactory[] = [],
index = 0
): Middleware {
const current = functions[index]
if (current !== undefined) {
const next = middlewares(functions, index + 1)
return current(next)
}
return () => ...
} const authenticate: MiddlewareFactory = (next) => {
return async (request, _next: ) => {
const pathname = request.nextUrl.pathname
if (['/admin']?.some((path) => pathname.startsWith(path))) {
const token = await getToken({ req: request })
if (token === null) {
throw new Error('You do not have permission to execute this action!')
}
if (!token.roles.includes('ROLE_ADMIN')) {
throw new Error('You do not have permission to execute this action!')
}
}
return await next(request, _next)
}
}
export default authenticate But the additional properties are useful in the action itself: // SignUp.tsx
export default function SignUp({ createUser }: Props) {
return (
<button onClick={async () => {
const result = await createUser({ name: 'Max' }, { subscribe: true });
}}>
Sign Up
</button>
);
} // action.ts
export const createUser = action(schema, async (data: Data, context: Context) => {
const { name } = data
const { subsctribe } = context
}); We can also consider such examples: // changeRole action.ts
export const changeRole = action(schema, async (data: Data, { id: string }): Promise<void> => {
await api.patch(`/admin/users/${id}/change-role`, data)
revalidatePath('/admin/users')
}) // activate action.ts
export const activate = action(id: string): Promise<void> => {
await api.patch(`/admin/users/${id}/activate`)
revalidatePath('/admin/users')
}) @JsSusenka, @TheEdoRan, maybe we should take this into account right away? |
I don´t think that you fully understood what the purpose of this PR was. In my example I´m already getting user role from user context. I don´t think that it is sufficient, to just get the required permissions from the request url. For example: // action.ts
export const deleteUser = action(schema, async ({ id }) => {
// an actual logic...
}, {
role: "admin", // role name
permissions: ["user.delete", "user.edit", "administrator"] // complex permissions
}); you can also pass complex permission array that is indeed very much useful when dealing with dashboards where you need fine-grained control over what users can do. @Myks92 feel free to adjust your solution, so it would fit my use case. |
In general, this is the case. But then we need to improve middleware. There are many unaccounted-for factors in the current offer. For example:
That's just part of what comes to mind. @JsSusenka it can be solved like this: //auth-action.ts
export const authAction = createSafeActionClient({
async middleware(parsedInput) {
const session = cookies().get('session')?.value
if (!session) {
throw new Error('Session not found!')
}
// In the real world, you would check if the session is valid by querying a database.
// We'll keep it very simple here.
const user = await getUserFromSession(session)
if (!user) {
throw new Error('Session is not valid!')
}
return { user }
}
}) // action.ts
export const deleteUser = authAction(schema, async ({ id }, context) => {
//context from middleware
const { user } = context
if (user.role !== 'admin') {
throw Error('Access denided')
}
if (!user.permissions.includes('user.delete')) {
throw Error('Access denided')
}
if (user.id !== id) {
throw Error('Access denided')
}
// an actual logic...
//context from action
const { redirect } = context
if (redirect) {
redirect(redirect)
}
});
deleteUser(schema, data, { redirect: "/admin" }) |
I would like to react on the points you mentioned above.
You indeed need to fetch the database to get the blog post based on the id that is being sent as a payload to the function. This point seems completely irrelevant to this PR.
The same issue happens in you first suggested approach. However, there are many ways of preventing this issue. For the sake of simplicity of this example, I had used plain string.
I certainly agree with this issue, but that has an easy solution. You can define the additional arguments like you would the schema outside of the parameters of the function and then pass the variable in to the function. I´ve provided an example down below. const additionalArguments = {
role: "admin", // role name
permissions: ["user.delete", "user.edit", "administrator"] // complex permissions
} // same as you define the action schema outside of the function to make it more readable
export const deleteUser = action(schema, async ({ id }) => {
// an actual logic...
}, additionalArguments );
Maybe if you can be more specific and add a example for this point, because I´m sorry, but I´m not getting what you mean by it. Since server actions tends to mutate state, querying the the user from the database on every request is very much needed, because you need to ensure that the user is logged in and that he possess the permissions to execute that action.
Don´t get me wrong, I sure can implement the role checking logic in every action that requires some elevated permissions than the basic logged in user has, but this approach is very tedious when dealing with large application. Please correct me if I´m wrong. |
If I may, I will comment only on this:
and SOLID will answer the rest. |
Lets say, that I divide my application according to the structure. Can you please provide a example on how would I handle permission checking in every action?
I´m sorry but I haven´t got a clue, what you meant by this point. Can you please explain it more or provide a small code example? This point seems to me like it is completely irrelevant to his PR once again.
This is exactly what middleware is used for. As you stated, I cloud have implementčerd the whole auth logic inside another function. This is exactly a middleware. This function will be called before every actual logic in the server action. Why should I implement it myself, when this library already offers very powerful way of handling middlewares? With the small change that is being introduced in this PR the middlewares become more powerful.
I don´t think you fully understand the root concept of server actions. You would call them only from client. Also this PR does not change the way you call actions from client. None of the additional arguments are being sent from client. These arguments are just defined right by the function in the file that is marked with the "use server" directive. Please read the docs of both server actions and this library itself so that we stand on even ground in terms of knowledge. Thank you
|
Hey @JsSusenka, thank for your code contribution, and also to @Myks92 for contributing to the discussion. I slightly adjusted your implementation to be type safe, renamed the I ultimately think this is a good feature, since it allows granular access control of actions via the middleware function. |
🎉 This PR is included in version 6.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi @JsSusenka @TheEdoRan, import { createSafeActionClient, type ServerCodeFn } from "next-safe-action"
import { type Schema } from "zod"
import { auth } from "~/auth/auth"
import { type Permission } from "~/auth/permissions"
const createActionAuth = createSafeActionClient({
async middleware(_parsedInput) {
const session = await auth()
if (session === null) {
throw new Error("Unauthenticated.")
}
return { session }
},
})
type AuthMiddleware = ActionMiddleware<ActionContext<typeof createActionAuth>>
const withPermissionProtection = (permission: Permission): AuthMiddleware => {
return (serverCode) => {
return (parsedInput, ctx) => {
if (!ctx.session.user.permissions.includes(permission)) {
throw new Error("Unauthorized")
}
return serverCode(parsedInput, ctx)
}
}
}
const createUser = createActionAuth(
userSchema,
withPermissionProtection("users.create")(
async (user, context) => { },
)
)
// Some utility types, useful to create a custom middleware
type ActionMiddleware<Ctx> = <const S extends Schema, const Data>(
serverCode: ServerCodeFn<S, Data, Ctx>,
) => ServerCodeFn<S, Data, Ctx>
type ActionContext<Action extends ReturnType<typeof createSafeActionClient>> =
Parameters<Parameters<Action>[1]>[1] Alternatively, to remove the differences between "main" and "additional" middlewares, we can create a wrapper on top of const createSafeActionClientWrapper = (options) => (middleware) = createSafeActionClient({ ...options, middleware })
const createSafeAction = createSafeActionClientWrapper({ handleReturnedServerError })
const withSession = (_parsedInput) => {
const session = await auth()
if (session === null) {
throw new Error("Unauthenticated.")
}
return { session }
}
const createUser = createSafeAction(
userSchema,
chainAsyncSequentially(withSession, withPermissionProtection('user.create')),
async (user, context) => {}
)
const deleteUser = createSafeAction(
z.string(),
// You're not obligated to run the middleware sequentially.
// Can use any chaining strategy https://github.com/sindresorhus/promise-fun?tab=readme-ov-file#packages
chainAsyncSequentially(withSession,
chainAsyncParallel(
withRoleProtection('Admin'),
withPermissionProtection('user.delete')
)
),
async (userId, context) => {}
) To conclude, you can use function composition for better code reusability. At least, this way your "main" middleware function is decoupled from the logic that doesn't belong to it (e.g. it doesn't know anything about permissions). Yes, the existing library API is not very functional programming–friendly by design, thus both your and my solutions are not ideal. Yours — in the way that it makes it difficult to compose middleware, and mine that it might require wrappers and boilerplate code to achieve that composable middleware. |
@vkhitev, thanks for the detailed usage example. I also believe that this feature does not apply to this package. I tried to convey this above, but they didn't hear me. For me, it's generally wild when something from the lower layer (action) suddenly transmits data to the upper layer (middleware). There is a standard from the Web world that describes the implementation in PHP. |
Moreover, I don't like the design of the library in that it takes over the validation of the data. This is a different responsibility and it needs to be done differently. // frontend/src/shared/validator
import { assert as typeSchemaValidator, type Schema, type InferIn } from '@typeschema/main'
/**
* @throws AggregateError
*/
export default async function validate<S extends Schema>(data: InferIn<S>, schema: S): Promise<void> {
await typeSchemaValidator(schema, data)
} // frontend/src/app/admin/users/create/action.ts
'use server'
import endpoint from '@/auth/api/endpoints/admin/users/create'
import action from '@/infrastructure/server-action/api-adapter'
import { revalidatePath } from 'next/cache'
import { redirect } from 'next/navigation'
import validate from '@/shared/validator'
import { z } from '@/shared/validator/adapter/zod'
interface Data {
lastName: string
firstName: string
email: string
password: string
}
const schema = z.object({
lastName: z.string(),
firstName: z.string(),
email: z.string(),
password: z.string()
})
export default action(async (data: Data, context: undefined): Promise<void> => {
await validate(data, schema)
const result = await endpoint(data)
revalidatePath('/admin/users')
redirect(`/admin/users/${result.id}`)
}) The package has become simple: // frontend/src/shared/action
import { isNotFoundError } from 'next/dist/client/components/not-found.js'
import { isRedirectError } from 'next/dist/client/components/redirect.js'
export type MaybePromise<T> = Promise<T> | T
export type Extend<Data> = Data extends infer U ? { [K in keyof U]: U[K] } : never
export interface Config {
handleError?: <Data>(e: Error) => MaybePromise<ActionError | ValidationErrors<Data>>
}
export interface Response<Data, Result> {
data?: Result
message?: ActionError['message']
errors?: ValidationErrors<Data>
}
export type ClientAction<Data, Context, Result> = (data: Data, context: Context) => Promise<Response<Data, Result>>
export type ServerAction<Data, Context, Result> = (data: Data, context: Context) => Promise<Result>
export interface ActionError { message: string }
export type ValidationErrors<Data> = Extend<Data>
export const DEFAULT_SERVER_ERROR = 'Server error'
export const isError = (error: unknown): error is Error => error instanceof Error
const create = (config?: Config) => {
return <Data, Context, Result>(action: ServerAction<Data, Context, Result>): ClientAction<Data, Context, Result> => {
return async (data, context) => {
try {
const result = ((await action(data, context)) ?? null) as Result
return { data: result }
} catch (e: unknown) {
/**
* next/navigation functions work by throwing an error that will be processed internally by Next.js.
* So, in this case we need to rethrow it.
*/
if (isRedirectError(e) || isNotFoundError(e)) {
throw e
}
if (!isError(e)) {
console.warn('Could not handle server error. Not an instance of Error: ', e)
return { message: DEFAULT_SERVER_ERROR }
}
return (await config?.handleError?.(e)) ?? { message: DEFAULT_SERVER_ERROR }
}
}
}
}
const action = {
create
}
export default action The next step is to make the first parameter an object: Contract: type StatusType = 'success' | 'error'
interface DefaultResult {
status: StatusType
message?: string
}
interface DataResult<TResult = unknown> extends DefaultResult {
status: 'success'
data: TResult
}
interface ErrorResult extends DefaultResult {
status: 'error'
error: string
message: string
}
interface ErrorsResult<TData> extends DefaultResult {
status: 'error'
errors: TData extends infer U ? { [K in keyof U]: U[K] } : never
}
export function isActionResult<TResult>(result: unknown): result is DataResult<TResult> {
return typeof result === 'object' && result !== null && 'data' in result && typeof result.data !== 'undefined'
}
export function isActionError(result: unknown): result is ErrorResult {
return typeof result === 'object' && result !== null && 'error' in result && typeof result.error === 'string'
}
export function isActionErrors<Errors>(result: unknown): result is ErrorsResult<Errors> {
return typeof result === 'object' && result !== null && 'errors' in result && typeof result.errors === 'object'
}
interface ActionRequest<TData = unknown, TParams = unknown> {
data: TData
params: TParams
}
type ActionResult<TResult = void, TErrors = unknown> = DataResult<TResult> | ErrorResult | ErrorsResult<TErrors>
type Action = <TData = unknown, TResult = void, TParams = unknown>(request: ActionRequest<TData, TParams>) => Promise<ActionResult<TResult, TData>> Action interface Data {
lastName: string
firstName: string
}
interface Result {
id: number
}
const schema = z.object({
lastName: z.string(),
firstName: z.string()
})
interface Params {
type: string
}
export async function action(request: ActionRequest<Data, Params>): Promise<ActionResult<Result, Data>> {
try {
await validate(request.data, schema)
} catch (error: unknown) {
return {
status: 'error',
error: 'ValidationErrors',
message: 'Validation errors'
} satisfies ErrorResult
}
if (request.params.type !== 'registration') {
return {
status: 'error',
error: 'RegistrationError',
message: 'Error'
} satisfies ErrorResult
}
return {
status: 'success',
data: { id: 1 },
message: 'User created'
} satisfies DataResult<Result>
}
const request = {
data: {
lastName: 'Last',
firstName: 'First'
},
params: {
type: 'registration'
}
} Usage const result = await create({
data: {
lastName: 'Last',
firstName: 'First'
},
params: {
type: 'registration'
}
})
if (isActionErrors<Data>(result)) {
console.log('Action errors result', result)
// { status: 'error', error: 'ValidationErrors', message: 'Validation errors', errors: [...]} }
}
if (isActionError(result)) {
console.log('Action error result', result)
// { status: 'error', error: 'ValidationErrors', message: 'Validation errors' }
} else {
console.log('Action data', result)
// { status: 'success', id: 1 }
} |
🎉 This PR is included in version 7.0.0-next.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hey @JsSusenka, @Myks92 and @vkhitev, thank you for sharing your thoughts on this feature. Please check out #88 and contribute to the discussion there, if you want. |
Hey guys :) What is the final recommandation to check a specific permission before the action execution ? Should I make a middleware for every permission I need to check ? Or should I find a way to create permissionGrantedAction which take an array of string (permission id/name) as a parameter ? Thank you for your help ! |
https://next-safe-action.dev/docs/recipes/extend-base-client |
No worries I have read the documentation :) This is my base client: safe-actions.ts:
And this is my authenticatedAction:
Should I try to make a single middleware with a permission parameter to check if the user has the permission like I tryed above ? |
This PR adds the ability to pass additional arguments from the action builder to the middleware. This is especially useful when you want to handle the auth logic inside the middleware and you need to check if user has specific roles or permissions to execute the function. I have created a small example of how this feature would be used.