Pundit: Docs recommend code that breaks spec example

[bbugh]

Hey, pundit is great so far. Thanks for doing a low-friction implementation to replace CanCan. I've written custom implementations quite a bit like pundit in the past and this has been saving us a lot of time.

We want to make sure that the user is logged in, and this "closed system" from the docs seems like a good idea. The docs suggest this:

class ApplicationPolicy
  def initialize(user, record)
    raise Pundit::NotAuthorizedError, "must be logged in" unless user
    @user = user
    @record = record
  end
end

However, the rspec permit extension doesn't seem to have an easy way to test this.

it 'fails without a user' do
  expect(subject).not_to permit(nil, record)
end

This doesn't work as expected - it throws an error. That's fine! However, I can't figure out how to trigger the permit matcher AND catch the error, except with ugly code like this.

it 'fails without a user' do
  expect {
    expect(subject).not_to permit(nil, record)
  }.to raise_error Pundit::NotAuthorizedError
end

which means we have to resort to making an instance of the describe policy class anyway, which defeats the purpose of using the rspec matcher. So either there's a better way to do what the documentation mentions in ApplicationPolicy (or it's a bad idea), or maybe the docs for how to test could be extended?

We're going to go back to using authenticate_user! in a before_filter anyway, but I thought this would be worthwhile to mention since it caused some confusion.

[darylprickett]

I do not have this issue with Rspec 3.1.0. The code that I use is the following:

it "allows visitor access to post" do
  expect(subject).to permit(nil, @post)
end

The @post is set in a before :each statement. Which version of Rspec are you using?

[EverybodyKurts]

@bbugh , @darylprickett I get the same error. @bbugh's workaround is the one i'm using.

[darylprickett]

@KurtRMueller What version of rspec are you using?

[weedySeaDragon]

@darylprickett: ditto what @KurtRMueller said: I get the error and am using the workaround. Here are the versions for my RSpec stuff:

rspec-core (3.2.1)
  rspec-support (~> 3.2.0)
rspec-expectations (3.2.0)
rspec-mocks (3.2.1)
rspec-rails (3.2.1)

[jnicklas]

I don't understand what the problem is expect { }.to raise_error is very idiomatic RSpec, is it not? I use it all the time.

[bbugh]

Nesting an expect inside of another expect is not idiomatic RSpec.

This should work and it doesn't, because it raises an error.

it 'fails without a user' do
  expect(subject).not_to permit(nil, record)
end

This hack is how it is required to function.

it 'fails without a user' do
  expect {
    expect(subject).not_to permit(nil, record)
  }.to raise_error Pundit::NotAuthorizedError
end

Either permit shouldn't throw an error if it's a failure, or there should be a helper for testing it. Right now, we have to wrap it inside of a double test.

This is the official way you've suggested to test, but it does not work for testing the "closed system" suggestion from the other section of the documentation, because in this case permit throws an error instead of returning true/false.

[jnicklas]

@bbugh your tone is not helpful, please be courteous, and let's not lose sight that what we're discussing here is a very minor niggle. If you have some idea on how the documentation can be improved, then please send a pull request.

Unfortunately there is no generic way in Ruby of making all methods in a class and all of its subclasses return false, and even if there were, would you really want to use something so hackish? So the exception based workflow is as good as close as you're going to get. The alternative is explicitly checking whether there is a user in all actions.

You could have written the test as:

it 'fails without a user' do
  expect { SomePolicy.new(nil, record) }.to raise_error Pundit::NotAuthorizedError
end

And then you wouldn't have needed the double-expect.

[bbugh]

I have no idea why you're ascribing emotion to my answer that wasn't there. You asked me a question and I answered it!

Thanks for the correct way to test this. While we're still on the original topic, can you modify the documentation to explain that? Other people are obviously having the same problem. :+1:

[ShaunSpringer]

This should be reopened and revisited. Testing policies is cumbersome with the method @bbugh has described.

Is there a proper (non-hacky) solution to this?

[KyleRAnderson]

Is it still the case that not using the included DSL is the best way to get around this as @jnicklas describes?
In my use case, I have permissions tied on a per-model basis, so even with an authenticated user I need to load the associated model permission to determine if they are authorized. So I do this in the constructor for the policy, and raise the NotAuthorizedError if it's nil (signifying that the user doesn't have an associated permission for the model).

[KyleRAnderson]

Actually I guess for these cases, I can just add a separate test case for it, since they would all fail in the constructor anyway, I don't need to check it by action.