-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
Update Authorizer types and add initial docs #191
base: main
Are you sure you want to change the base?
Conversation
> & { | ||
context?: DataFunctionArgs["context"]; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context shouldn't be option, I know you may not use it, and it's the most common thing, but the type is not optional in LoaderArgs so it shouldn't be here, or at least not the types the rules receives, we could accept context as optional but pass it to the rules with a default empty object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've updated it to be optional in the call to authorize
but the rule functions will always receive a context (an empty object if not provided).
): Promise<User> { | ||
if (!raise) raise = "response"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the default raise should be error, I think that's more expected and if the developer setup Sentry it should know if this throw and the dev is not catching it.
Also, Authorizer may receive the default raise and use it as default here
For consistency, the idea is that if the user doesn't pass the validation of two different rules the rule throwing should always be the same, the first one, and then after that's solved the second one, otherwise you will receive different errors every time you reload.
This sounds like a good idea. |
I still think the error thrown should be an interface RuleFunction<User, Data> {
(context: RuleContext<User, Data>): Promise<boolean>;
}
type Rule<User, Data> =
| {
message: string;
rule: RuleFunction<User, Data>;
}
| RuleFunction<User, Data>; It wouldn't break any existing use cases and would not be a breaking change. It would mean a small change to the checking of rules to something like: for (let rule of [...this.rules, ...rules]) {
let callableRule = typeof rule === "function" ? rule : rule.rule;
let message =
typeof rule === "function"
? `Forbidden${rule.name ? ` by policy ${rule.name}` : ""}`
: rule.message;
if (await callableRule({ user, ...args, context: args.context ?? {} })) continue;
// omitted
Then if you need a custom error message for a rule you can just use the object version of the rule like so: await authorizer.authorize(
{ request, params: { id: "1" } },
{
rules: [
{
rule: async ({ user }) => user.role !== "admin",
message: "some message",
},
],
}
); This is probably simpler than my initially proposed alternative which would result in being able to only use the error raise option, not being able to use arrow functions, having to wrap the call to authorize in a try/catch, check the thrown error is instanceof AuthorizationError and then check if a string value is equal to the name of one of your rule functions. A separate question: since |
I initially attempted to update the Remix dev dependencies but that caused two kinds of errors in the tests:
createCookieSessionStorage
is no longer exported directly from@remix-run/server-runtime
but from@remix-run/node
et al which was an easy fix.new Request
with relative paths seemed to be failing with an invalid path error, however that isn't the typical behavior so I'm unsure of what the issue was.Authorizer Updates
raise
is assigned. This was required to get the types working properly and remove the expect errors.DataFunctionArgs
is made optional to avoid having to provide the entire loader/action args if you don't use context in your app. Let me know if that doesn't work for you.Docs Updates
Authorizer
in the docs folder.Questions
AuthorizationError
to include the rule name if applicable and used in the authorizer (instead of the genericError
)? Catching and checking for that error could make an easy way to set your own error messages for specific rules.