feature request: Add matcher for raise_not_authorized

[jpgeek]

One pattern for Pundit with closed systems is to throw a Pundit::NotAuthorizedError on initialize when the user is nil. It would be nice to test this using the lovely pundit-matchers type syntax:

 ` it { is_expected.to raise_not_authorized(:index) }`

I am happy to send a pr.

[chrisalley]

We were looking at treating exceptions as a bug fix for the existing permit/forbid matchers: #16

Perhaps we can simply update permit_action and forbid_action to support exceptions as a failure state. However, if we want to explicitly check than an exception was raised within the policy action, perhaps a chained syntax, e.g. it { is_expected.to forbid_action(:index).by_raising_not_authorized }

[jpgeek]

How should that work? Something like forbid_action succeeds for either NotAuthorizedError or authorization denied, and if .by_raising_not_authorized is appended, it should fail unless that error is raised. This would break backward compatibility though, right?

Alternatively, forbid_action could continue to succeed for unauthorized but blow up for NotAuthorizedError UNLESS it is specified with .by_raising_not_authorized. Not intuitive though.

#16 seems reasonable - to have one matcher for "Nope", NotAuthorizedError or otherwise. Maybe another matcher for forbid_action_or_raise_not_authorized - kinda verbose though.

For me, a separate raise_not_authorized would be clearest but I understand what you are saying on it.

[chrisalley]

I'm in favour of a solution where the tests are concerned with whether the action is permitted/forbidden rather than how the action is permitted or forbidden. The question of whether it was an exception that denied the action feels like an implementation detail rather than something that would be useful to test explicitly.

That said, there are concerns about backward compatibility. This could justify a major version release.

[jpgeek]

@chrisalley , that is certainly a valid way of looking at it. For the application where this came up for me, I handled forbidden and Pundit::NotAuthorizedError differently; the former as a flow that might happen normally and the latter as an exceptional case which should not. As you say, at the integration or feature level it is an implementation detail, but at the unit level, the response from Pundit and the difference between forbidden and an exception seems in scope, as the consumer of that message has to respond differently. That is why a separate raise_not_authorized makes sense for me.

Anyway, your call and I appreciate all you have done and are doing on this project!

[chrisalley]

How about an API where the differing ways of being unauthorised are additional matchers, while forbid_action on it's own handles all types of failure?

• Forbid the action by returning false or raising a Pundit::NotAuthorizedError error:
  it { is_expected.to forbid_action(:index) }

• Forbid the action by returning false:
  it { is_expected.to forbid_action(:index).by_returning_false }

• Forbid the action by raising a Pundit::NotAuthorizedError error:
  it { is_expected.to forbid_action(:index).by_raising_not_authorized }