Protector.insecurely does not nest

[crosebrugh]

During debug I put this around blocks in several places. I monkey-patched:

module Protector
  class << self

    # allow nesting
    def insecurely(&block)
      Thread.current[:protector_disabled] ||= 0
      Thread.current[:protector_disabled] += 1
      yield
    ensure
      Thread.current[:protector_disabled] -= 1
    end
  end
end

module Protector
  module DSL
    module Base

      # better error message
      def protector_subject
        unless protector_subject?
          raise "Unprotected entity detected: use .restrict to protect it:\n  #{self.class}.#{self.id || 'new'}"
        end

        @protector_subject
      end

      # Checks if model was restricted
      def protector_subject?
        @protector_subject_set == true && (Thread.current[:protector_disabled] == 0)
      end
    end
  end
end

[inossidabile]

I like these changes. Would you like to make a PR?

[crosebrugh]

Bummed that I've had to back Protector out of our code. I sure like its DSL but just ran in to too many issues trying to retrofit it into our app. Still getting restrict issues even within Protector.insecurely so there are still some holes to plug. As usual, pragmatism wins and I'll have to stick with our home-brewed stuff.

On Sun, Sep 15, 2013 at 9:48 PM, Boris Staal notifications@github.comwrote:

I like these changes. Would you like to make a PR?

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24488814 .

[inossidabile]

Answering your initial question, Protector was not designed to have mode allowing you to "emulate everything is accessible". Protector.insecurely make the system think an entity is unprotected. So it behaves same – it raises an exception where it thinks you shouldn't be asking for a permission unless you specified a context.

What's the scenario where you wanted to have "always-true" permissions?

[crosebrugh]

Thanks for explaining. It would be cool if you put that in the README.

I use .insecurely for seeding data (builtin users, roles, etc) as well as for debugging - just needing to turn everything off.

On Sun, Sep 15, 2013 at 10:07 PM, Boris Staal notifications@github.comwrote:

Answering your initial question, Protector was not designed to have mode allowing you to "emulate everything is accessible". Protector.insecurelymake the system think an entity is unprotected. So it behaves same – it raises an exception where it thinks you shouldn't be asking for a permission unless you specified a context.

What's the scenario where you wanted to have "always-true" permissions?

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24489163 .

[inossidabile]

Ye, well, that was clear from the start :). I need a little bit more precise explanation of the case where you want can? to always return you true for seeding or testing purpose. To me it feels like you are doing something wrong.

[crosebrugh]

We seed (from code) at least a builtin super user and a handful of builtin roles. When protector is active we get exceptions since there's no user yet. As mentioned, I expected .insecurely to turn off all Protector logic.

Yes, we could guardband the protect do ... end logic with some Thread.current[] logic but that's a pretty kludgy workaround for such a common use case.

[inossidabile]

That's a kludgy workaround indeed. But how do you even get your models restricted at seeds? Why don't just work with plain old models? This clearly is the place where they shouldn't be restricted at all.

[crosebrugh]

Not sure I understand the questions.

We programmatically create! the super user/roles and we need all authorization logic turned off when doing so. During normal usage we need the logic on so users cannot create those. So if by "plain old models" you mean models with no protection then that doesn't work since protection is needed during normal usage.

On Tue, Sep 17, 2013 at 10:18 AM, Boris Staal notifications@github.comwrote:

That's a kludgy workaround indeed. But how do you even get your models restricted at seeds? Why don't just work with plain old models? This clearly is the place where they shouldn't be restricted at all.

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24605917 .

[inossidabile]

Doesn't look like I totally understand what you mean saying seeds. I thought you were ralking about rake db:seed. Can you please give me more details about how those seeds run? Is that a controller or..? How do you know if that's a time to seed super user? Etc. Thank you so much for your answers btw. I know you are not using Protector but it can really help somebody else :+1:

[crosebrugh]

I appreciate the dialog as well.

It is triggered by rake db:seed. Here's an example of our seed.rb:

ActiveRecord::Base.transaction do
  begin
    Role.seed
    User.seed
  rescue => e
    Rails.logger.error(e.message)
    raise e
  end
end

app/models/role.rb

class Role < ActiveRecord::Base
  # do not change these internal names once seeded!
  SUPER="super"
  BUSINESS_ADMIN="business_admin"
  SITE_ADMIN="site_admin"
  USER="user"
  GUEST="guest"

  ALL={
    SUPER => 70,
    BUSINESS_ADMIN => 60,
    SITE_ADMIN => 50,
    USER => 20,
    GUEST => 10
  }

  has_many :users, :inverse_of => :role

  default_scope { order('level ASC') }

  validates :name,
  :presence => true,
  :uniqueness => true,
  :length => { :maximum => MAX_NAME_LENGTH }

  validates :internal_name,
  :presence => true,
  :uniqueness => true,
  :length => { :maximum => MAX_INTERNAL_NAME_LENGTH }

  validates :level,
  :presence => true,
  :uniqueness => true,
  :numericality => { only_integer: true, greater_than_or_equal_to: 0 }

  # allow Role[Role::BUSINESS_ADMIN]
  def self.[](r)
    Role.find_by_internal_name(r.to_s)
  end

  def self.seed
    ALL.each do |internal_name,level|
      unless Role[internal_name]
        Role.create!(internal_name: internal_name,
                     name: internal_name.titleize,
                     level: level)
      end
    end
  end
end

[inossidabile]

So if I understand it right it gets restricted internally at self.seed. So why don't you just pass a flag into seed like seed(protect=true)? And that's it? Then if you call it directly without specifying anything – it protects by default. If you call it from rake db:seed you pass false into it (cause it doesn't make any sense to restrict things from a rake task)

[crosebrugh]

It seems like we're going in a circle here. How do I turn off all protection within ,seed without a kludge? Aren't we back to my original point that I expected .insecurely to turn everything off?

On Tue, Sep 17, 2013 at 11:22 AM, Boris Staal notifications@github.comwrote:

So if I understand it right it gets restricted internally at self.seed. So why don't you just pass a flag into seed like seed(protect=true)? And that's it? Then if you call it directly without specifying anything – it protects by default. If you call it from rake db:seed you pass false into it (cause it doesn't make any sense to restrict things from a rake task)

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24610828 .

[inossidabile]

Let me try to explain it from the start. I believe that the piece of code should either:

• Always work with protected models
• Always work with unprotected models
• Manually check the state of protection in complex cases using .protector_subject?

Everything else is a source of potential bugs that are VERY likely to stay unnoticed. So in seed you don't have to "turn off" all protection. You should not turn it on. Something somewhere calls restrict! on the models. Make it skip this part if some kind of a flag is set. You are running Model.seed from your seeds – pass a flag into it forcing it to not restrict a model.

[crosebrugh]

I don't understand how .protector_subject? comes in to play. In my use case, how, - exactly - do you apply that? It doesn't seem that that turns off any protection.

On Tue, Sep 17, 2013 at 11:36 AM, Boris Staal notifications@github.comwrote:

Let me try to explain it from the start. I believe that the piece of code should either:

• Always work with protected models
• Always work with unprotected models
• Manually check the state of protection in complex cases using .protector_subject?

Everything else is a source of potential bugs that are VERY likely to stay unnoticed. So in seed you don't have to "turn off" all protection. You should not turn it on. Something somewhere calls restrict! on the models. Make it skip this part if some kind of a flag is set. You are running Model.seed from your seeds – pass a flag into it forcing it to not restrict a model.

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24611963 .

[crosebrugh]

Another use case for something like Protector.ignore is when writing specs it's common to create/modify records in the DB in order to setup a test case.

Authorization gets in the way of setting up the desired state, so to be able to do that within a Protector.ignore block seems legit.

On Tue, Sep 17, 2013 at 11:45 AM, Chris Rosebrugh crosebrugh@gmail.comwrote:

I don't understand how .protector_subject? comes in to play. In my use case, how, - exactly - do you apply that? It doesn't seem that that turns off any protection.

On Tue, Sep 17, 2013 at 11:36 AM, Boris Staal notifications@github.comwrote:

Let me try to explain it from the start. I believe that the piece of code should either:

• Always work with protected models
• Always work with unprotected models
• Manually check the state of protection in complex cases using .protector_subject?

Everything else is a source of potential bugs that are VERY likely to stay unnoticed. So in seed you don't have to "turn off" all protection. You should not turn it on. Something somewhere calls restrict! on the models. Make it skip this part if some kind of a flag is set. You are running Model.seed from your seeds – pass a flag into it forcing it to not restrict a model.

— Reply to this email directly or view it on GitHubhttps://github.com/inossidabile/protector/issues/26#issuecomment-24611963 .