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

Merging explicit context with implicit context #173

Closed
brendon opened this issue Jul 19, 2021 · 8 comments
Closed

Merging explicit context with implicit context #173

brendon opened this issue Jul 19, 2021 · 8 comments
Labels
investigate Something is probably wrong

Comments

@brendon
Copy link
Contributor

brendon commented Jul 19, 2021

Tell us about your environment

**Ruby Version: 3.0.1

**Framework Version (Rails, whatever): 6.1.3.1

**Action Policy Version: 0.5.7

Reproduction Script: TBC - I just want to confirm if this is a documentation error first.

What did you do?

I passed an explicit context to allow_to?.

What did you expect to happen?

I expected the context that I set explicitly to be merged with the implicit context (As in I'm overriding one key).

What actually happened?

The context seems to be overwritten by the explicit one. In my case the policy execution was unable to find the user from the context until I supplied it explicitly.

This seems to contradict the documentation:

NOTE: the explicitly provided context is merged with the implicit one (i.e. you can specify only the keys you want to override).

per: https://actionpolicy.evilmartians.io/#/authorization_context?id=explicit-context

@brendon
Copy link
Contributor Author

brendon commented Jul 19, 2021

This commit seems to have changed the behaviour. At a quick glance it's hard for me to decide if the intended behaviour was just moved off to: 8c4b66f#diff-9e75a9746d75303b84e1fcb1576ecaac33d1ad9696dff6c37ece6515a8f9b74aR55

8c4b66f

This logic now seems to be placed here:

options[:context] && (options[:context] = authorization_context.merge(options[:context]))

@brendon
Copy link
Contributor Author

brendon commented Jul 19, 2021

Digging deeper I think this might have turned into a caching issue. I'm doing this kind of thing:

  def view_blob?
    allowed_to?(:view?, record) ||
    allowed_to?(:view?, record, context: {context: :admin})
  end

In most instances I can supply the context (subkey) automatically, but in this case I need to specifically try a different context because I don't know the context by the way the request is made.

@palkan
Copy link
Owner

palkan commented Aug 20, 2021

I just want to confirm if this is a documentation error first

Documentation is correct: explicit context MUST be merged into implicit. So, it would be great to have a reproduction script 🙂

this might have turned into a caching issue

Hm 🤔 The commit you mentioned earlier was meant to fix this; there could be a bug, but I cannot notice it in the commit; so, having an example code would be helpful.

@palkan palkan added the investigate Something is probably wrong label Aug 20, 2021
@brendon
Copy link
Contributor Author

brendon commented Aug 21, 2021

Thanks @palkan, I'm very happy to make a test case. I tried running this: https://github.com/palkan/action_policy/blob/master/.github/bug_report_template.rb

But it fails on missing routes. Before I go digging around, is this a bug in the test suite? I don't see any routes declared in there, so I guess they just need to be added. Strange that it would have ever run through :D

@brendon
Copy link
Contributor Author

brendon commented Aug 21, 2021

Wait, I see the routes there. I'll dig deeper :)

@brendon
Copy link
Contributor Author

brendon commented Aug 21, 2021

Ok, here you go. I had to constrain the rails gem to 6.0 as 6.1 needs some rework with this template:

https://gist.github.com/brendon/ddd1bf9bbe26e08b21c176b0539dd0e7

You can see it raise ActionPolicy::AuthorizationContextMissing: Missing policy authorization context: user when we try to augment the context.

@palkan palkan closed this as completed in 5e9fff3 Aug 27, 2021
@palkan
Copy link
Owner

palkan commented Aug 27, 2021

Thanks for the reproduction! Fixed.

@brendon
Copy link
Contributor Author

brendon commented Aug 29, 2021

Nice :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Something is probably wrong
Projects
None yet
Development

No branches or pull requests

2 participants