Custom errors

[br0p0p]

Hi, I'm getting familiar with this library and am finding it very useful so far.

One thing I would like to be able to do is customize the default errors somewhat. It's pretty straightforward to provide an error message when forbidding a single action using because but in many cases it's preferable to allow many actions for a user and base error messages on the user itself and not the user's action when they are not authorized.

To give some background, we have a GraphQL API and we would like the flexibility to customize the default error messages in certain situations. One reason is so we can only call the throwUnlessCan to authorize and not have to catch the error from throwUnlessCan and then throw our own in each resolver. Another is that we want to be able to hide certain information from the users' when appropriate (internal models or "subjects", etc.)

Example:

// For every resolver we have an `authorize` function:
// We'd prefer to do this:

const authorize = async (root, args, context) => {
	context.ability.throwUnlessCan('read', 'Post'); // throws custom error - Error('Not authorized!')
}

// instead of manually catching and then throwing for every resolver

const authorize = async (root, args, context) => {
	try {
		context.ability.throwUnlessCan('read', 'Post'); // catch default error
	} catch(error) {
		throw new Error('Not authorized!'); // throw our own custom error
	}
}

I don't see a way to achieve this currently but if I just missed it, let me know.

As far as I can tell, our needs could be met by allowing extension of the ForbiddenError and/or accepting a custom error via the AbilityBuilder/Ability constructor(s) that would be used in place of the static ForbiddenError here:

casl/packages/casl-ability/src/ability.js

Lines 198 to 204 in 9ec8b17

	throw new ForbiddenError(rule ? rule.reason : null, {
	action,
	subjectName,
	subject,
	field
	});
	}

I'm happy to contribute toward this goal if needed.

Thanks for your help!

[stalniy]

Hi

Thanks for the issue. I’m not sure whether it make sense to do this on casl level.

If the only thing you want to change is what GraphQl client will receive then I’d suggest to use formatError (https://www.apollographql.com/docs/apollo-server/data/errors/) function of Apollo

[stalniy]

If it’s not what you need then please provide an example of situation where you want to change errors and how (based on user role I guess)

[br0p0p]

formatError seems to be more about making sure errors play well with GraphQL and making sure the client receives errors in a useable format. It's doable in formatError but that only applies in a GraphQL context. I think what I'm asking for is more generally useful.

The way I see it working from the dependent would be something like this. It's a contrived example but I think this illustrates my use case.

import { AbilityBuilder, ForbiddenError } from '@casl/ability'

// Could be a function like this that returns a 
function makeError(reason, { action, subjectName, subject, field }) {
	let message = reason;

	if (action === 'delete') {
		message = "Not authorized.";
	}

	return new ForbiddenError(message, { action, subjectName, subject, field });
}

// Add an option for AbilityBuilder to customize the error behavior
const ability = AbilityBuilder.define({ makeError }, allow => {
	allow('read', 'Post');
})

// uses custom error logic
ability.throwUnlessCan('delete', 'Post'); // throws "Not authorized."
ability.throwUnlessCan('update', 'Post'); // throws default error message: 'Cannot execute "update" on "Post"'

I think this would make things pretty straightforward in the library internals:

// Default makeError implementation - used if not provided by user
function makeError(reason, { action, subjectName, subject, field }) {
	return new ForbiddenError(reason, {
        action,
        subjectName,
        subject,
        field
	});
}

// Only minor changes in throwUnlessCan implementation
// Use function to create error in `throwUnlessCan` 
throwUnlessCan(...args) {
	const rule = this.relevantRuleFor(...args);

  	if (!rule || rule.inverted) {
    	const [action, subject, field] = args;
	    const subjectName = this[PRIVATE_FIELD].subjectName(subject);
		const makeError = this[PRIVATE_FIELD].makeError;

		throw makeError(rule ? rule.reason : null, {
      		action,
		    subjectName,
      		subject,
      		field
    	});
  	}
}

[stalniy]

OK, I see. Your point is to be able to hide information about action/subject based on some conditions.

Actually I've been thinking how can I move throwUnlessCan out of Ability class (because on UI side it's very likely a dead code which would be good to shake out with help of webpack/rollup), that's why I don't want to extend this point. Maybe it's a good time to discuss this can look and how it will fullfill your requirements.

So, what I have in mind right now:

import { throwIf } from '@casl/ability'

// 1st option, requires ErrorBuilder
thowIf(ability).can('read', 'Post');
thowIf(ability).cannot('read', 'Post');

// 2nd option, simpler but then we don't know checking details (i.e., subject, action, field). So, this is probably not viable
throwIf(() => ability.can('read', 'Post'))

What I like about both is verbosity. Both can support your usecase, what you will need is to implement own throwIf function.

So, in the 1st option (suppose casl expose new ErrorBuilder class):

import { ErrorBuilder } from '@casl/ability'

class MyCustomError extends Error {}

export default function throwIf(ability) {
   return new ErrorBuilder(ability, MyCustomException)
}

ErrorBuilder may have some configuration like:

new ErrorBuilder(ability)
  .use(ForbiddenError) // use as default Error
  .use(MyCustomError, { onlyIf: (action, subject, field) => /* custom logic */ })

Alternatevely you can extend ErrorBuilder and override a method which is responsible for throwing error.

2nd option even simpler:

import { ErrorBuilder } from '@casl/ability'

class MyCustomError extends Error {}

export default function throwIf(condition) {
   if (condition) {
      throw new MyCustomException() // the issue here is that there is no easy way to get ability checking details
   }
}

What do you think? Maybe you have other suggestions in this direction?

[br0p0p]

Idk, throwUnlessCan seems helpful to have client-side in some cases but a throwIf to replace it seems more flexible and useful in both server and client.

---

I really like this option from an API perspective:

// 1st option, requires ErrorBuilder
thowIf(ability).can('read', 'Post');
thowIf(ability).cannot('read', 'Post');

This API flows well with how the rest of the library is used but personally I'm not a fan of having to write my own throwIf implementation. I would rather augment how the provided throwIf works in certain situations.

I would prefer doing something like one of these options instead of implementing my own throwIf:

// functional style
thowIf(({ subject, action, field }) => {...}), ability.can('read', 'Post'));
// or if you'd prefer it this way the params could be the other way around, putting throwing handler last, I just prefer functional style
thowIf(ability.cannot('read', 'Post'), ({ subject, action, field }) => {...});

// also possible to use throwIf without the custom handler param to use default error logic:
throwIf(ability.can('read', 'Post'));
throwIf(ability.cannot('read', 'Post'));

// if you don't like the custom error handling options above (passing functions to `throwIf`, you could also do something like this which seems very consistent with your current approach:
throwIf(ability.cannot('read', 'Post')).because('Only certain users can read posts');
throwIf(ability.cannot('read', 'Post')).because(({ subject, action, field }) => `Only certain users can ${action} posts`);

The last suggestion makes a lot of sense to me because when initially defining allowed/forbidden abilities, you are essentially saying "always allow/forbid this action for this reason" but when checking abilities, it seems very useful to be able to say "in this case the action is forbidden and throws for this reason" and could throw for a different reason elsewhere. The because conveys this meaning well I think.

These options seem more composable and flexible to me but I'm not familiar with the codebase and you might have implementation-related reasons for doing it a certain way. Just my two cents from a user perspective.

[stalniy]

Thanks for the suggestions, they are really helpful!

This one:

throwIf(ability.cannot('read', 'Post')).because(({ subject, action, field }) => `Only certain users can ${action} posts`);

cannot be achieved without saving information about last checking subject/action/field inside ability instance. I'd like not to do this.

So, if you want to provide own message without adding custom implementation I can do this:

import { throwError } from '@casl/ability'

throwError.onlyIf(ability).cannot('read', 'Post') // default error will be thrown
throwError('Only certain users can read posts').onlyIf(ability).cannot('read', 'Post') // custom error

I'll be waiting for the feedback

Update: also it can be rephrased like:

throwError.unless(ability).can('read', 'Post')
throwError('Only certain users can read posts').unless(ability).can('read', 'Post') // custom error

Update 2:
No need to overcomplicate it can be like this

throwError().unless(ability).can('read', 'Post') // custom error
throwError('Only certain users can read posts').unless(ability).can('read', 'Post') // custom error

[br0p0p]

Nice! I like this for per action error throwing:
throwError('Only certain users can read Posts').unless(ability).can('read', 'Post')

Something my first comment achieves but the later suggestions do not (at least that I can tell) is top level error customization. For instance, if I wanted to replace all "Cannot execute <action> on <subject>" error messages with a simple "Not authorized." could we add functionality to allow that?

I'm glad to help with any changes to make these things happen once the API is decided.

[stalniy]

If I move error throwing out of Ability class, then the only way to change error message is something like this:

import { ForbiddenError } from '@casl/ability'

ForbiddenError.defaultMessage = () => "Not Authorized"

[stalniy]

After some thinking I have doubts whether it make sense to create such a complicated API just to provide different error message :)

So, maybe instead it will be enough to have this:

import { ForbiddenError, authorize } from '@casl/ability'

// The next line solves the issue with providing error messages
ForbiddenError.defaultMessage = ({  subjectName, action, field }) => action === 'delete' ? 'Not Authorized' : `Cannot execute ${action} on ${subjectName} ${field}`

// and this line throws an error if there is no ability to read Post
authorize(ability, 'read', 'Post', 'title')

// and implementation of authorize will be the same as `throwUnlessCan` 

This is simpler because I will not need to write another class. But the downside is that it's not possible to provide error message in specific places. That logic needs to be centralized in ForbiddenError.defaultMessage.

[stalniy]

Also I don't like that:

throwError().unless(ability).can(...)

Has no real meaning if we split these functions, I mean what I use throwError() without calling the next methods in chain:

throwError('test')

From reader perspective, it should throw an error but it returns ErrorBuilder and it's confusing.

[br0p0p]

Does the because ever stand on its own? I think the same thought process applies there so I'm not sure conveying meaning when splitting the functions is a big concern. How the functions are chained is a different problem but I think the library already relies on chaining functions that may not have individual meaning.

---

I think you understand my needs:

• Allow per-action custom error messages when checking abilities (extend throwUnlessCan type of functionality). This is the most important request for me and seems like it should exist given that because exists in ability creation.
• Allow error customization centrally when creating the ability. More of a convenience thing - not necessary but nice to have.

As far as how these requests are met and how the API looks, you should be the one to decide since you're the most familiar with the library. So whatever you think is best is good with me. :)

[stalniy]

Ok, the eventual api will be this:

ForbiddenError.from(ability)
  .setMessage(“Custom message”)
  .throwUnlessCan(“read”, “Post”) 

// to set global error 
ForbiddenError.setDefaultMessage(() => “Default error message”)

This way I won’t bring new entities into the code and people will continue using familiar throwUnlessCan

update: for tree shaking tools like webpack and roll up this is also good because both ForbiddenError and throwUnlessCan will be removed from the code if not used.

[stalniy]

available in @casl/[email protected]

[br0p0p]

thanks for your work on this!