Skip to content

Fix: Pundit adapter blindly ignores existing pundit_user definition#2467

Closed
theangryangel wants to merge 1 commit into
railsadminteam:masterfrom
theangryangel:fix/pundit_user
Closed

Fix: Pundit adapter blindly ignores existing pundit_user definition#2467
theangryangel wants to merge 1 commit into
railsadminteam:masterfrom
theangryangel:fix/pundit_user

Conversation

@theangryangel

Copy link
Copy Markdown

As per the pundit readme, there are instances where I need to provide additional context about the user. I do this by defining pundit_user, however the pundit rails_admin adapter ignores this and replaces it with it's own pointing at current_user. This breaks policies that rely on the additional context.

This patch addresses this.

In addition to this I have another issue with the pundit adapter, and I'd like to include some fixes for it as well, but before just doing them I'd like to see if there's a good reason why things have been done the way they are.

Typically the method name for checking with pundit for "can I do X" will be suffixed with a "?". Currently the pundit adapter does not do this.

I see 2 problems with this:

  1. In scenario Missing foreign key displays as null in the list view #1, it means aliasing a bunch of methods (i.e. alias_method :index, :index?) - a minor incovenience
  2. In scenario Values that are null in the list view show a URL #2, I may want different permissions for rails_admin than my standard application policies, for some reason. Currently I'm able to do this because it calls different methods (those without a question mark), but it's not particularly clear that these are specifically for rails_admin. This has confused a junior developer here already.

I'd like to put a patch together that basically either prefixes the methods with rails_admin, or works more like sudosu/rails_admin_pundit where it runs through a specific method, which in turn can use a case statement.

I'd appreciate any input on that before I write a patch and start depending on it.

… definition. This allows additional context as per the pundit readme.
@mshibuya

Copy link
Copy Markdown
Member

For the scenario 1, that's my fault. I'll change all pundit calls to be suffixed with ?.

For the scenario 2, the recommended way is to set custom authorization key through action configuration.
#2399 (comment)

@theangryangel

Copy link
Copy Markdown
Author

Thanks for the pointer, @mshibuya. Much appreciated <3

jibidus pushed a commit to sogilis/rails_admin that referenced this pull request Nov 26, 2015
jibidus pushed a commit to sogilis/rails_admin that referenced this pull request Nov 26, 2015
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.

2 participants