Option to cancel relationship update authorization

[mkamensky]

Hi,

In the creation example (for instance), when create_with_author? does not exist, the fallback is to check update? for the user. I find this unreasonable: when creating the article I'm not really updating the author. I know I could change the behaviour by defining a create_with_author? method, but I would need to do it for every association in all my models (and when adding new ones)

My suggestion: allow specifying a fallback or a method name in place of update? (with update? as default), which is independent of the relationship name (e.g., create_with?)

Thanks!

[valscion]

Hi, and thanks for raising the issue!

when creating the article I'm not really updating the author

I beg to disagree with this. If the author model has association has_many :articles, technically you are modifying the author record in that case as the_author.articles will have changed when you created the new record.

My suggestion: allow specifying a fallback or a method name in place of update? (with update? as default), which is independent of the relationship name (e.g., create_with?)

Hmm. For context, the update? fallback logic happens in three places right now:

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 119 in da04d6c

::Pundit.authorize(user, record, 'update?')

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 255 in da04d6c

::Pundit.authorize(user, source_record, 'update?')

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 258 in da04d6c

::Pundit.authorize(user, related_record, 'update?')

One idea that I could get behind is extracting these lines (and only these lines) to a new method on DefaultPunditAuthorizer. Users of this gem could then create a new subclass of DefaultPunditAuthorizer and configure their application to use that authorizer instead.

The new method could be something like

class DefaultPunditAuthorizer
  def fallback_authorize(user, record)
    ::Pundit.authorize(user, record, 'update?')
  end
end

I'd be open to review such a PR, with tests attached, and willing to consider including it in the gem itself. A mention in the documentation somewhere + in-line comments would be necessary to tell about this possibility.

[mkamensky]

Hi and thanks for the quick reply

I beg to disagree with this. If the author model has association has_many :articles, technically you are modifying the author record in that case as the_author.articles will have changed when you created the new record.

This is a valid philosophical point, but the practical implication is that I cannot allow someone to create or update an article without also allowing them to update the author. In my case, the dependency graph between all models in the project is connected, so I will be allowing them to update everything...

At any rate, I assume this is pointless to discuss, since changing it will break existing code. I will try implement what you suggest when I can get to it. In the meantime, I have workaround overriding the respond_to? method, but this is not something I'd like to keep...

Thanks!

[valscion]

Hi and thanks for the quick reply

🙇. Even though there hasn't been many commits in the near history, I still try to answer issues and pull requests in a timely manner. This gem is alive and breathing just fine ☺️

This is a valid philosophical point, but the practical implication is that I cannot allow someone to create or update an article without also allowing them to update the author.

Well, you can do that by creating the special methods in your policies. But I get your point ☺️.

In my case, the dependency graph between all models in the project is connected, so I will be allowing them to update everything...

That definitely isn't good, and isn't what we'd want people to do.

At any rate, I assume this is pointless to discuss, since changing it will break existing code.

I'm trying not to sound nasty here — this project just has gone through this same topic a few times in the past already. I'm actually quite glad that you were able to reference the relationship authorization documentation, because at least the current logic wasn't too surprising to you 😄

I will try implement what you suggest when I can get to it.

Let's hope you'll get to it at some point, or someone else who wants to work on this can also do so. If you or someone has questions on code or something, I'd try to help to the best I can.

The one thing I won't do is code this feature on my own, as we don't need it at work right now. Here's a great chance to do some open source goodness 😉

In the meantime, I have workaround overriding the respond_to? method, but this is not something I'd like to keep...

Yikes, sounds flaky! I hope contributing code to this gem would even be easier for you in the long term ☺️

[mkamensky]

I'm trying not to sound nasty here — this project just has gone through this same topic a few times in the past already. I'm actually quite glad that you were able to reference the relationship authorization documentation, because at least the current logic wasn't too surprising to you 😄

Actually, I first found it in the code, then found it was documented :) Anyway, thanks again, I hope I can send something soon.

[valscion]

Great! Feel free to ask even stupid questions 🤗. After all, I am sure to have blind spots in what is difficult to grasp as I'm already knee-deep in the code itself.

[mkamensky]

Just had another look. Won't it be better to use method names that do not include the relationship type (as in create_with?)? This is more flexible and works better with inheritance, and it is very easy to implement the current behaviour.

[mkamensky]

Just had another look. Won't it be better to use method names that do not include the relationship type (as in create_with?)? This is more flexible and works better with inheritance, and it is very easy to implement the current behaviour.

Or, even easier, instead of calling respond_to? on the policy object, just call the method, and rescue the exception if the method is not found by what is now the else block. This is completely backward compatible (at least with respect to the docs), and allows one to use method_missing to deal with the call

[valscion]

"More flexible" in this case also means that it's easier to accidentally allow some operation that should not have been allowed. The philosophy of this gem, "err on the side of too much authorization than too little", is what is happening here. It is far too easy to create a create_with policy method and forget to handle an edge case.

What is entirely possible is to come up with an application-side abstraction for avoiding too much repetition in policy classes. I'd be quite curious to hear about them, and maybe it would even be beneficial to document them.

As for the second idea, calling a method that most likely does not exist and rescue an error, I'd rather not do it. Exceptions should be reserved for exceptional cases, not control flow. And also encouraging a method_missing? would also make it too easy to allow an edge case authorization