Hello, as per the git blame, that change has been introduced intentionally because of #6884

Hello, as per the git blame, that change has been introduced intentionally because of #6884

Hmmm, interesting. I didn't find that in my searches.

Do you have any suggestions about what we could do here? I now see there are scenarios where this makes sense but also scenarios where it is a bad idea (e.g. #8969).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.08%. Comparing base (8239e4c) to head (22d064e).
⚠️ Report is 2 commits behind head on master.

@@           Coverage Diff           @@
##           master    #8986   +/-   ##
=======================================
  Coverage   99.08%   99.08%           
=======================================
  Files         139      139           
  Lines        4046     4046           
=======================================
  Hits         4009     4009           
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The fact that specs are not failing is odd, it means that the fix is not testing correctly what it was supposed to fix

The fact that specs are not failing is odd, it means that the fix is not testing correctly what it was supposed to fix

The only spec in the merged PR just checks that it applies it, which I removed.

which I removed.

Sorry, it's a small PR, but didn't notice that when I've commented.

I mean, it was checking something, but is not testing what that scope is needed for (creation of nested objects?)

However, in the original issue #8969 I can see that it is advised to not apply authorization scope on new and create, so yes, as are you mentioning, none of the two is correct

Copilot suggests to exclude some keys from the params. Should we exclude the primary key? Is this related to the primary key?

The cause of our specific issue was scoping to id but it could apply unexpectedly to anything. In the other example I provided for instance:

You set your scope to be Post.where(published: true) - you only want your users to see published posts
When they create a post, it's automatically set published: true due to the authorisation scope moving to the new record, which seems very counter intuitive.

The comment in the pundit adapter suggests it is not meant to be used for new and create, but here we are using it.

I don't think excluding keys is the answer - it would mean you'd need to know in advance of this unexpected behaviour and plan for it, and know to add new ones as things change, which is easy to miss. I don't know what the correct solution is now though!

Personally, I think the PR that introduced this may have needed more consideration before merging, but now it is established functionality that some rely on, it would seem.