in development & test errors caused by failing operations should be meaningful

[catmando]

currently we don't give a lot of info back on failures when you do a find on a record that doesn't exist or when you don't have proper permissions. This is to keep the security footprint as small as possible.

But in development and test, we should provide a nice report in the JS log.

[mareczek]

In production this feedback is needed even more than in development. There should be a way of handling 404 and 401 for find-like methods. How do you now do show components with handling a RecordNotFound?

[janbiedermann]

500: for failing ServerOps i noticed that the exception appears in the response body, because the exception is caught by the Promise and the Promise is returned: { ‚error‘: ‚‘good‘ method not found’ } What about monkey patching Promise in dev and test, so its possible to set a global flag $BREAK_PROMISES = true and Promise will no longer cover the exceptions and instead fail hard.

class Operation
  step do
    $BREAK_PROMISES = true
    # code that fails and needs debugging
    $BREAK_PROMISES = false
  end
end

Alternatively maybe the Exception could be tee'd to the console.

[sfcgeorge]

Yeah, debugging permissions is actually very difficult without the model name.

Firstly, the error class defines message but elsewhere just to_s is called so you don't see the error class in the output: {"error":"for view_permitted?([\"id\"])"}. So either whatever outputs this error should be calling .message or we should also define to_s as in this monkeypatch:

module ReactiveRecord
  class AccessViolation < StandardError
    def message
      "ReactiveRecord::AccessViolation: #{super}"
    end

    def to_s # <== add
      "ReactiveRecord::AccessViolation: #{super}"
    end
  end
end

Second issue is not including the model name. I think it's not leaking any information as you already compile the models into Opal so the names can already be seen on the frontend. This patch easily includes the model name:

class ActiveRecord::Base
  def check_permission_with_acting_user(user, permission, *args)
    old = acting_user
    self.acting_user = user
    if self.send(permission, *args)
      self.acting_user = old
      self
    else
      raise ReactiveRecord::AccessViolation, "for #{model_name} #{permission}(#{args})" # <== change
    end
  end
end

So the output is now {"error":"ReactiveRecord::AccessViolation: for Message view_permitted?([\"id\"])"}. Much better.

If you're worried about the security of users inferring which permissions they have in production then something like:

# ...
      message = Rails.env.production? ? "" : "for #{model_name} #{permission}(#{args})"
      raise ReactiveRecord::AccessViolation, message
# ...