Use `scope.none` by default in generated templates

[tagliala]

Hi! 👋🏼 👋🏼

First time contributing here

I'm not sure if this change:

• should be considered a breaking change
• the new behavior should be documented somewhere
• every supported ORM responds to none

Let me know what do you think

---

A01:2021-Broken Access Control is the category with the most serious web application security risk.

Using scope.all in templates violates the principle of least privilege or deny by default, where access should only be granted for particular capabilities, roles, or users.

This change improves the security of default templates

Ref: https://owasp.org/Top10/A01_2021-Broken_Access_Control/

[Burgestrand]

Hi! Thank you for the PR!

This is a bit of a cultural change, so I'd like to wait with merging this until @dgmstuart has had the time to take a look as well.

I like this; if nothing else it will make our users stop for a second and think about access, like a security speed bump to make sure we slow down. It's likely that the majority of time the implementation will be changed to .all anyway, but as a general rule I believe developers should strive to prevent human error, err on the side of caution, and to practice good habits.

There's a possible problem in people blindly using the generated code, and then believing there's a bug somewhere because suddenly they're not getting any records any more. It's a bit invisible and could even be hard to track down. What do you think about also raising an exception, maybe something like this:

def resolve
  raise NotImplementedError, "you need to implement #resolve"

  scope.none
end

The thinking with the exception is that it'll blow up very visibly and point the developer to the piece of code they need to take a look at before continuing on.

should be considered a breaking change

I'm pretty confident that this is not a breaking change. If people update their Pundit gem they won't need to change any of the code they've written before updating, so we're good.

the new behavior should be documented somewhere

There's a Unreleased section in CHANGELOG.md, please add a "Changed" header within the Unreleased section and then a note about this change. Include the pull request number (#711). That should be good enough. :)

[dgmstuart]

@tagliala I'm not sure about this - will have to give it a think.

My concern is that its ActiveRecord, not Pundit which establishes the convention that an empty scope is equivalent to all, and I don't want to be fighting against the framework unless it's really worth it.

eg:

scope :foo, -> {}

...then foo is equivalent to all, which is the "null" value of a scope (at least in ActiveRecord).

I'm more open to it being defined as .none in ApplicationPolicy, since that lines up with all the action? methods being false, but I'm not so sure it makes sense to specify it in the policies which inherit from that.

@Burgestrand it doesn't make sense to me to both raise NotImplementedError and include .none as the "default" implementation, when that default is already defined in the parent. I think it should be either

• implement something here (probably including .all), OR
• delete this whole method (and therefore use the default behaviour defined in ApplicationPolicy)

I think the smell here is that this "default" is set in both templates (that's been the case for a long time - not something introduced here): I'm pretty sure that's a bad idea and if we can make that go away then everything will sit more nicely together.

What do you think about removing the resolve method from the Policy template entirely? That would work with both new and old installations. Personally I never use the generators and rarely define policy scopes, so it's hard to judge what the impact of that would be.

Alternatively:

# lib/generators/pundit/install/templates/policy.rb 
def resolve
  super
end

🤷

[tagliala]

Hi,

thanks for your thorough answers!

Personally I never use the generators and rarely define policy scopes, so it's hard to judge what the impact of that would be.

Same here. I do not use policy generators

What do you think about removing the resolve method from the Policy template entirely?

My thoughts:

• Removing the resolve method sounds good; but
• Using super may work better for both documentation purposes and faster configuration (even if you don't normally write a method like that)

[Burgestrand]

@dgmstuart @tagliala I believe we should still define a resolve in the Policy template, with the same reasoning as before. If we don't include it and a developer uses policy_scope then by default your code will not work, and it will not raise an error, so it'll be difficult to find why it doesn't work and that's time we can save everyone by having a default implementation in the Policy template that raises an error.

# lib/generators/pundit/install/templates/policy.rb 
def resolve
  raise NotImplementedError, "you need to implement this before using the scope"
end

That said, I'm willing to give whatever solution a try. I believe it's better to get this out the door and then possibly change it if necessary. :)

[dgmstuart]

If we don't include it and a developer uses policy_scope then by default your code will not work

Isn't using policy_scope without explicitly defining scope in the policy pretty unlikely? What would be the point? GIGO.

I'd challenge the idea that "not returning any records" means "not working": if you do use policy_scope without defining a scope I'm not sure we can presume to know what your expectations are around how that should work.

# lib/generators/pundit/install/templates/policy.rb 
def resolve
  raise NotImplementedError, "you need to implement this before using the scope"
end

On second thoughts have a few concerns about this:

1. It suggests that scopes should be defined for all policies, and I don't believe that's true: in practice I've mostly not ended up using policy_scope. For me scopes are an optional feature and one that has turned out to be kind of problematic 😅.
2. It suggests that scopes need to be defined explicitly for all policies: most of the time when you see raise NotImplementedError it's a signal that the code won't work unless you define this function. That's not the case here.
3. My understanding has always been that raise NotImplementedError is an anti-pattern. It's widely used, but that's not because it's a good idea: it's often a sticking plaster over abuses of inheritance. I haven't been able to find a good source to back that up with though, so maybe I'm just wrong about that.

Honestly I'm leaning towards leaving the implementation as it is rather than making a change which relies on this raise.

[tagliala]

I'd challenge the idea that "not returning any records" means "not working":

Strongly agree. As already mentioned, it is something like def index?; false; end and also plays better with other methods defaulting to false, so I guess that users should change the default policy to make it work

Honestly I'm leaning towards leaving the implementation as it is

I didn't get this, you mean with scope.all by default in both template policies?

[dgmstuart]

Honestly I'm leaning towards leaving the implementation as it is

I didn't get this, you mean with scope.all by default in both template policies?

Yes: if we conclude that the trade-off of changing it to scope.none in ApplicationPolicy, is that we have to add a NotImplementedError then personally I don't think that trade-off is worth it.

it is something like def index?; false; end

It seems similar, but I'm going to argue that it's not the same: if you call a controller action which isn't doesn't have an explicit corresponding policy rule (and is therefore default false/unauthorized), you'll get a NotAuthorizedError that nudges you to add a specific policy rule.

@Burgestrand's point is that, in contrast, with policy_scope there's no exception.

Also: with the policy rules, usually a specific Policy will only override a couple of them, so it makes sense to set a list of defaults.

But with policy_scope there's only one thing, so for every policy it's either not used or we're expecting it to be overridden. That' means we're never expecting the default to be called, unless something has gone wrong.

I'm not sure there's any valid use case in which you're using policy_scope without having overridden resolve, and my confidence in that only gets stronger if the default is scope.none: why would you ever want to call something which will always resolve to scope.none?

So if we're expecting all calls to it to be overridden, isn't it somewhat academic what the default value is?

Or how about (and I know this is wildly inconsistent of me) we double-down on the idea that the whole concept of a default is invalid and have resolve raise a NotImplementedError in ApplicationPolicy, and then delete Scope from the individual policy generator altogether?

[Burgestrand]

Hi @tagliala! We talked about this a lot, and came up with a slightly different approach but one which we believe addresses the concerns that you raised in this PR. It's very much based on your work, and we tried to properly attribute the original concern to you as best we could.

This PR will be closed once we merge #722.