accessible_by may crash when a default_scope is used

[ghiculescu]

Problem

If you have a default_scope that sets ordering, and define a permission that checks for records via multiple tables, and use a has_many :through, and run Postgres... then on version 3.x, accessible_by may crash. The attached test suite shows how; the tests that call alex.blog_post_comments fail with:

     ActiveRecord::StatementInvalid:
       PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
       LINE 1: ... ORDER BY "blog_post_comments"."created_at" DESC, "blog_post...

This regressed in #512

Previous query (worked fine):

SELECT "blog_post_comments"."id" AS t0_r0, "blog_post_comments"."blog_post_id" AS t0_r1, "blog_post_comments"."body" AS t0_r2, "blog_post_comments"."created_at" AS t0_r3, "blog_post_comments"."updated_at" AS t0_r4, "blog_posts_blog_post_comments"."id" AS t1_r0, "blog_posts_blog_post_comments"."blog_author_id" AS t1_r1, "blog_posts_blog_post_comments"."title" AS t1_r2, "blog_posts_blog_post_comments"."created_at" AS t1_r3, "blog_posts_blog_post_comments"."updated_at" AS t1_r4, "blog_authors"."id" AS t2_r0, "blog_authors"."name" AS t2_r1, "blog_authors"."created_at" AS t2_r2, "blog_authors"."updated_at" AS t2_r3 FROM "blog_post_comments" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" WHERE "blog_posts"."blog_author_id" = 1 AND "blog_authors"."id" = 1 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

New query (crashes):

SELECT DISTINCT "blog_post_comments".* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" WHERE "blog_posts"."blog_author_id" = 1 AND "blog_authors"."id" = 1 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

After writing this up I realised there was a much simpler replication case reported here: #508 (comment)

Also I know that this is mentioned in the updating notes but in my opinion cancancan should be able to handle this for the user.

---

Solution

Okay so I kind of hate this PR but I haven't found a better solution. There's a few things going on here.

• add_joins_to_relation. Relation#left_joins does not work super well in AR 5.0 and 5.1; it's failing on a bunch of the tests added in this PR. Thus I'm not using it for older Rails versions; for those I'm falling back to how joins worked before Kaspernj optimised queries with left joins #512.
• extract_ids_and_requery has like a million comments explaining what it does. It means we'll be running an extra query fairly often so I'm keen for feedback on if this is actually worth doing.

Basically I hate this solution but I can't find another way of getting it to work if we keep left_joins. And if we ditch left_joins, there's still things that don't work. For example, there's some tests that do Post.accessible_by(ability).select('title as mytitle') that only work if you use left_joins (they were introduced after #512).

[ghiculescu]

@kaspernj @coorasse what do you think?

[ghiculescu]

Based on the CI output it looks like #512 also introduced an issue Rails 5.0 and 5.1 when defining abilities with nested hashes?

If you reverse the changes made in #512 then the new tests here I've added all pass. Because of this, I'm proposing we only use left_joins from AR 5.2 onwards: 1997832. Annoying, this is causing plenty of other things to fail.

Before I continue down this path I'm keen for feedback on if the 2 changes I'm proposing in this PR are worthwhile, and if it makes more sense to split them into separate PRs. Here's the changelog for reference: a9642b2

[kaspernj]

Great work! I am very happy you have found my code useful, and I cant wait to use this instead of my own gem :-)

[ghiculescu]

@kaspernj @coorasse keen for some opinions on this one. See updated PR body.

[coorasse]

Hi @ghiculescu . Thanks for the time you put in this PR.
I quickly tried to pull your branch locally and run all the tests you added, removing the patch you applied.
The result is that the tests
rspec ./spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb:147 and
rspec ./spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb:140 fail because of the following error:

ActiveRecord::StatementInvalid:
       SQLite3::SQLException: ambiguous column name: blog_posts.blog_author_id: SELECT DISTINCT "blog_post_comments".* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" ON "blog_posts"."id" = "blog_post_comments"."blog_post_id" WHERE "blog_posts"."blog_author_id" = ? AND "blog_posts"."blog_author_id" = ? ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

I therefore see an error in the examples you provided but which is not related to the default_scope. The same error appears if I remove the default_scope.
The problem is due to a poor management of joins and left_joins when used at the same time (as you highlighted already) because of names clashing (no alias is added to one of the joins).
At the same time I can also see that in AR >= 5.2 the aliases are added and the tests pass but the left join could be avoided.

Here are my considerations at the moment:

• I think a better test to reproduce the error is necessary. By removing the patch and running the tests I could see any PG::InvalidColumnReference error triggered.
• I think an improvement could be made for all the versions when a join is already present (read: do not generate another left join). This is another matter and can be tackled in a separate PR. this could also solve the issue on version 5.0

Can you help me to reproduce your issue? If I have missed something please point to me. I do not absolutely exclude that I might have missed something. 😄

Thanks 🙏

[coorasse]

One additional note regarding your comment:

Also I know that this is mentioned in the updating notes but in my opinion cancancan should be able to handle this for the user.
There are so maaaany different situations that I think it would be better to tackle them with code examples rather than trying to manage them all.
At some point a nested join solves all the issue 😄

[ghiculescu]

@coorasse here's how to replicate: https://gist.github.com/ghiculescu/a3a713134f18e351c7ba521377146014

Take the tests I've added in this PR and nothing else, and run DB=postgres bundle exec appraisal activerecord_5.2.2 rspec spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb.

Note that the tests don't fail if you're using sqlite. But I use postgres in production, that's how I came across the issue.

[ghiculescu]

@coorasse any more thoughts on this one?

If you aren't happy with this style of fix I can go back and work around this issue in my code. Keen to get a resolution one way or another as this is blocking our rails 6 update.

[ghiculescu]

We have ended up working around this in our codebase.