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

Update Authorizer types and add initial docs #191

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

Conversation

myleslinder
Copy link
Contributor

@myleslinder myleslinder commented Sep 8, 2022

I initially attempted to update the Remix dev dependencies but that caused two kinds of errors in the tests:

  1. createCookieSessionStorage is no longer exported directly from @remix-run/server-runtime but from @remix-run/node et al which was an easy fix.
  2. 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

  • The only runtime code that was changed is the way the default value to raise is assigned. This was required to get the types working properly and remove the expect errors.
    • If you'd prefer I not re-assign to the argument I can modify to use a local variable instead.
    • The types should now be set up to infer the data property type from the first rule provided including if that rule is a route specific rule and there are no global rules.
  • The context property of the 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.
  • I've ran the tests and checked the types in the tests and everything seems to be working

Docs Updates

  • I've put together an initial attempt at some bare-bones docs for Authorizer in the docs folder.

Questions

  • Is there a reason the rules are called sequentially as opposed to in parallel?
  • There isn't really a good way to know which rule caused the authorization to fail. Would you be open to augmenting the existing AuthorizationError to include the rule name if applicable and used in the authorizer (instead of the generic Error)? Catching and checking for that error could make an easy way to set your own error messages for specific rules.

Comment on lines +11 to +13
> & {
context?: DataFunctionArgs["context"];
};
Copy link
Owner

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.

Copy link
Contributor Author

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";
Copy link
Owner

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

@sergiodxa
Copy link
Owner

Is there a reason the rules are called sequentially as opposed to in parallel?

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.

There isn't really a good way to know which rule caused the authorization to fail. Would you be open to augmenting the existing AuthorizationError to include the rule name if applicable and used in the authorizer (instead of the generic Error)? Catching and checking for that error could make an easy way to set your own error messages for specific rules.

This sounds like a good idea.

@myleslinder
Copy link
Contributor Author

myleslinder commented Sep 13, 2022

I still think the error thrown should be an AuthorizationError however since it's really just about being able to provide specific error messages when certain rules fail, how would you feel about instead allowing the rules provided to be either a RuleFunction or an object with a RuleFunction and a message property, like so:

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

I have the real version of this implemented locally with some additional tests and all the previous tests continue to pass

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 isAuthenticated can now take a session instead of a request and authorize already has the request to get the session off of, would it make sense to flash the error message to the session in cases where the raise option is "redirect"?

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.

None yet

2 participants