Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependent policy loses track of dependee failure reason #50

Closed
adisandro opened this issue Nov 22, 2018 · 4 comments
Closed

Dependent policy loses track of dependee failure reason #50

adisandro opened this issue Nov 22, 2018 · 4 comments

Comments

@adisandro
Copy link

Running from master, with ruby 2.5.3 and rails 5.2.1.

If I have two policies, one of which is dependent on the other:

class DependeePolicy
  def do_it?
    check?(:a?) && check?(:b?)
  end

  def a?
    true
  end

  def b?
    false
  end
end
class DependerPolicy
  def do_it?
    check?(:do_it?, record.dependee) && check?(:c?)
  end

  def c?
    true
  end
end

Sometimes I authorize the Dependee on its own:

begin
  authorize! dependee, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # { :dependee => [:b?] }
end

Sometimes I authorize the Depender instead, but:

begin
  authorize! depender, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # { :dependee => [:do_it?] }
end

I would expect the reason to still be { :dependee => [:b?] }, or a chain of failures { :depender => [:do_it?], dependee => [:do_it?, :b?] }. We lose track on the underlying real reason otherwise, especially for i18n messages.

(Awesome gem by the way!)

@palkan
Copy link
Owner

palkan commented Nov 23, 2018

Yeah, that's a tricky thing.

I have an idea which solves this particular case (but not sure that's it's clear):

  • when check?(:smth) is called on the same record/policy, we only add { current_policy => [:smth] } to reasons
  • when check?(:smth, other_record) is called, we "unwrap" sub-policy reasons if any, i.e. { sub_policy => [:some_reason] }; if no reasons then { sub_policy => [:smth] }.

Example:

class DependeePolicy
  def do_it?
    check?(:a?) && check?(:b?)
  end

  def do_that?
      b? || c?
  end

  def a?
    true
  end

  def b?
    false
  end

  def c?
    false
  end
end

class DependerPolicy
  def do_it?
    check?(:do_it?, record.dependee) && check?(:c?)
  end

  def do_that?
    check?(:do_that?, record.dependee) || check?(:d?)
  end

  def c?
    true
  end

  def d?
    false
  end
end

# first case: self-check
begin
  authorize! dependee, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # { :dependee => [:b?] }
end

# 1: self-check
begin
  authorize! dependee, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # { :dependee => [:b?] }
end

# 2: no checks
begin
  authorize! dependee, to: :do_that?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # {}
end

# 3: sub-policy check with sub-checks
begin
  authorize! depender, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  puts e.result.reasons.details # { :dependee => [:b?] }
end

# 3: sub-policy check without sub-checks
begin
  authorize! depender, to: :do_that?
rescue ActionPolicy::Unauthorized => e
  # NOTE: we use dependee rule as reason, 'cause no reasons were populated in sub-call
  puts e.result.reasons.details # { :dependee => [:do_that?], :depender => [:d?] }
end

@adisandro
Copy link
Author

Apologies for the late reply.
What you propose make sense, except maybe for example 2. Shouldn't it return { :dependee => [:do_that?] } too? (i.e. the "external" authorize! acting like a check? call?)
I may be wrong here because I don't have a policy like that in our code, so I haven't tried.

@palkan
Copy link
Owner

palkan commented Dec 3, 2018

What you propose make sense, except maybe for example 2. Shouldn't it return { :dependee => [:do_that?] } too?

Reasons are only populated when check? or allowed_to? is used.

External authorize! populates e.result.policy, e.result.rule and e.result.message (which would contain DependeePolicy and :do_that?).

The idea of reasons is to specify the failure cause more precisely and not to replace the top-level "reason" (i.e. a rule that failed).

We use both e.result.message and e.result.reasons.full_messages` for displaying errors, like that:

begin
  authorize! dependee, to: :do_it?
rescue ActionPolicy::Unauthorized => e
  msg = e.result.message

  if e.result.reasons.any?
    msg << ": #{e.result.reasons.full_messages.map(&:downcase).join(', ')}"
  end

  puts msg
end

@adisandro
Copy link
Author

Thank you for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants