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

Allow to reset authorization context #265

Closed
inkstak opened this issue May 23, 2024 · 7 comments
Closed

Allow to reset authorization context #265

inkstak opened this issue May 23, 2024 · 7 comments

Comments

@inkstak
Copy link
Contributor

inkstak commented May 23, 2024

Tell us about your environment

Ruby Version: 3.2.1

Framework Version: Rails 7.0.8.1

Action Policy Version: 0.6.9

Reproduction Script: https://gist.github.com/inkstak/ee0d954288aae6b624ae5d7b68bb8748

What did you do?

I need 2 values in my authorization context : current_user & current_account
But before assigning a current_account, I first need to check if I can switch to it

So, in the same controller request, I have to call two policies :

  1. The first one is a authorization checks before assigning the current_account.
    At this time, current_account is not yet available in the current context :

    authorize! account, to: :switch?, with: AccountPolicy # context: { user: current_user }
    @current_account = account
  2. The second one is a "regular" check.
    Like most of authorization checks, it requires a current_account to be available in the current context :

    authorize! to: :index?, with: UserPolicy # context: { user: current_user, account: current_account }

What did you expect to happen?

I should be able to do a first check without account context and perform any subsequent checks with account context.

What actually happened?

When doing the first check, ActionPolicy memoizes the context without account.
When it comes to the second check, the account is still missing from the memoized context :

ActionPolicy::AuthorizationContextMissing (Missing policy authorization context: account):

What workarounds have you tried?

My current workaround is to reset memoization of the current context after setting current_account:

@current_account = account
@_authorization_context = nil

.. but it's not ideal as it depends on a private instance methods

I've also tried to explicitely pass the context in the first check :

authorize! account, to: :switch?, with: AccountPolicy, context: { user: current_user }

... but somehow, the explicit context seems to be memoized anyway

What do you suggest?

I have 2 suggestions :

The first and easier one is, like my workaround, to add a public method to reset context :

def reset_authorization_context
  @_authorization_context = nil
end

The second would be to skip memoization when an explicit context is passed.
However, I cannot imagine the downsides or side effects...

@inkstak
Copy link
Contributor Author

inkstak commented May 23, 2024

I found the answer to the second suggestion :

context = context ? authorization_context.merge(context) : authorization_context

The method authorization_context and memoization are always called in order to merge the explicit context.
It's a documented behavior : https://actionpolicy.evilmartians.io/#/authorization_context?id=explicit-context :

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

The first suggestion is the only one remaining 😥

@inkstak inkstak changed the title Allow to reset authorization context or avoid memoize it when passing explicit one Allow to reset authorization context ~or avoid memoize it when passing explicit one~ May 23, 2024
@inkstak inkstak changed the title Allow to reset authorization context ~or avoid memoize it when passing explicit one~ Allow to reset authorization context May 23, 2024
@ezekg
Copy link

ezekg commented May 23, 2024

@inkstak
Copy link
Contributor Author

inkstak commented May 23, 2024

@ezekg Thanks, I've already found this issue before posting this one (I should have referenced it 🙇)
But I didn't want to disable memoization : memoization is great and usefull to all the checks coming after the first set_account ... which happens on every requests.

@palkan
Copy link
Owner

palkan commented May 30, 2024

The second would be to skip memoization when an explicit context is passed.

memoization are always called in order to merge the explicit context.

Yeah, we want to support the case when you want to override only a part of the context via the context: option. That's why memoization is still being used.

I wonder if we should skip memoization in case an explicit context is provided 🤔 Sounds like a viable option, IMO (I don't use the context: option too often though). That means, whenever the context: option is passed, we implicitly disable memorization for such calls. @inkstak @ezekg Thoughts?

@inkstak
Copy link
Contributor Author

inkstak commented May 30, 2024

I wonder if we should skip memoization in case an explicit context is provided

That was my first (or second) thought but since it was implemented with memoization and documented as such, It will introduce breaking changes.

I also thought about another workaround without using private variables, which is to update the current context:

authorize! to: :switch?, account, with: AccountPolicy # context: { user: current_user }

authorization_context[:account] = account

authorize! to: :index?, with: UserPolicy # context: { user: current_user, account: current_account }

@palkan
Copy link
Owner

palkan commented May 30, 2024

it was implemented with memoization and documented as such, It will introduce breaking changes

I don't think we have authorization context memoization documented, so it should be rather safe change.

@inkstak
Copy link
Contributor Author

inkstak commented May 31, 2024

You're right, memoization is not mentionned.
Since I knew that implicit context imply memoization, I overinterpreted this quote from https://actionpolicy.evilmartians.io/#/authorization_context?id=explicit-context

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

PR is on the way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants