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

Enable Pundit authorization with namespaced decorators #7934

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rogerkk
Copy link
Contributor

@rogerkk rogerkk commented Apr 24, 2023

I guess the testing could do with some love, and perhaps we should add tests for both namespaced and non-namespaced decorators. Am I on the right track here?

What

When retrieving auth policies and the subject is wrapped in a namespaced decorator, Pundit is not able to find the policy. My original issue with full description and code to reproduce is in issue #7933.

How

This fix makes use of ResourceController::Decorators.undecorate to undecorate the target before asking pundit to fetch the policy.

It does this in PunditAdaper#policy_target, so as to have the fix affect PunditAdapter#retrieve_policy which in turn is used by PunditAdapter#authorized.

Unless I'm missing something the remaining public methods are not affected by the issue at hand.


Fixes #7933

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e242b89) 99.10% compared to head (3a74ef8) 99.10%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7934   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         140      140           
  Lines        4017     4019    +2     
=======================================
+ Hits         3981     3983    +2     
  Misses         36       36           

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

@lukeasrodgers
Copy link

fwiw @rogerkk this would fix a similar issue I'm having with the CanCanAdapter, have been working around it by implementing custom authorized? logic that uses subject.try(:decorated?) but I think using undecorate is better

@rogerkk
Copy link
Contributor Author

rogerkk commented Jun 27, 2023

@lukeasrodgers Ah, thanks for the verification!

If you want to have a stab at making codecov happy, I'll be happy to share the glory ;) If not I'll see if I can set off some time do it and see if it's possible to get the attention of a maintainer.

@rogerkk rogerkk marked this pull request as ready for review July 27, 2023 10:17
@rogerkk
Copy link
Contributor Author

rogerkk commented Jul 27, 2023

Changing state of this PR from a draft, in the hopes of attracting maintainer attention. 😅

Is there any interest in getting this into master? If so then I can put a little effort into improving the tests, rebasing and all that jazz.

@rogerkk rogerkk changed the title Make sure Pundit auth policy target is undecorated Enable Pundit authorization with namespaced decorators Oct 11, 2023
@rogerkk rogerkk closed this Jan 10, 2024
@rogerkk rogerkk reopened this Jan 11, 2024
@rogerkk
Copy link
Contributor Author

rogerkk commented Jan 11, 2024

Still eager to get a fix for this into master

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.

Authorization fails with Admin namespaced decorator
2 participants