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

In permitted_attributes, switch from database column detection to Rails attributes detection. Addresses #838 #839

Merged
merged 5 commits into from
May 23, 2024

Conversation

kalsan
Copy link
Contributor

@kalsan kalsan commented Dec 1, 2023

This pull request addresses #838 and makes Cancancan "see" attributes that are not backed by the database. It enables code like the following:

class Foo < ApplicationRecord
  attribute :bar
end

With this pull request, cancancan's permitted_attributes will "see" and include :bar even when there is no corresponding column in the database.

Reasonning why this is a good idea: When implementing a form and checking for per-attribute authorization, there are often more fields in the form than are actually stored. An example would be a checkbox "I agree to the terms of use" which is checked by the controller and then discarded if checked, validation error otherwise. If cancancan ignores this attribute (since it's not DB backed), a form whitelisting attributes by using cancancan's permitted_attributes would miss on this field.

This becomes particularly important when using cancancan together with Gems like active_type.

…mn detection to Rails attributes detection. Fixes #838"

This reverts commit 530a202.

Reason: Adjust tests to obtain a before and after picture
…ser in order to include Rails' actual column loading behavior
…s, switch from database column detection to Rails attributes detection. Fixes #838""

This reverts commit 1195d39.
@kalsan
Copy link
Contributor Author

kalsan commented May 19, 2024

Hi @coorasse, it was a pleasure meeting you IRL :-)

Thank you for your suggestions regarding testing. You've asked me to implement a failing "before" test, as well as a passing "after" test. However, it's a bit trickier than that because of the following situation:

  • The current test verifying that column names are read correctly by CanCanCan creates a method column_names on an ActiveRecord::Base and checks that they are read back correctly.
  • However, this method does not affect attribute_names which is inconsistent with Rails' actual behavior: when loading the columns, Rails automatically adds each column under attribute_names, see also this StackOverflow question regarding the topic.
  • Therefore, while this PR works in real life, the existing test would fail.

Therefore, I modified the PR to consist of three commits:

  • 1edfe71 modifies the test to actually perform testing on the DB instead of working only in-memory, enabling the needed Rails behavior of propagating column names to attributes.
  • 4936716 modifies the test such that it will fail with the current implementation, demonstrating that attributes are not correctly handled in 3.5.0.
  • f68d752 adds the fix that makes the test pass. This is the commit this PR is for.

I hope that these suggested code changs are to your liking and that the PR can be merged to improve CanCanCan as discussed :-)

Have a nice prolonged week-end!
Best,
Kalsan

Copy link
Member

@coorasse coorasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks!

@coorasse coorasse merged commit 0cdbba7 into CanCanCommunity:develop May 23, 2024
16 of 24 checks passed
@kalsan kalsan deleted the support-rails-attributes branch May 23, 2024 16:32
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

Successfully merging this pull request may close these issues.

None yet

2 participants