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

[Update Readme Only] Add namespace policy example #497

Closed

Conversation

wayne5540
Copy link

Thanks for the hard work on this amazing project. It's would be great if we add an example to specify how to override customize policy finder so that we can do a controller based authorization. references: #12, #178

I've posted my solution on issue12, I think it would be great to list on README.md as well since I'm not the only one want to do controller based authorization. 😃

@wayne5540 wayne5540 changed the title Add namespace policy example [Update Readme Only] Add namespace policy example Nov 20, 2017
@wayne5540 wayne5540 mentioned this pull request Nov 20, 2017
@wayne5540 wayne5540 force-pushed the add-policy-override-readme branch from 31683a1 to 7e9df73 Compare November 20, 2017 14:13
@maxshelley
Copy link

I don’t think you need to over-ride the internal method to achieve what you're looking to do. Does passing the namespace in as a symbol when you need to check the policy achieve the same result?

With a Admin::PhotosController here's how that would look:

  class Admin::PhotosController < ApplicationController
    def show
      authorize [:admin, @photo], :show?
    end
  end

Which would look for app/policies/admin/photo_policy.rb.

That would look like:

  class Admin::PhotoPolicy < ApplicationPolicy
    def show?
      # perform auth logic e.g. user.admin?
    end
  end

This approach seems to be working for us.

If you didn't want to specify the namespace each time and would rather infer it from the namespace of the controller, you could add something to the application controller that would generate the array of symbols and then pass in the object you're authorizing. I haven't had a chance to test this, but I think it would look like:

class ApplicationController < ActionController::Base

  # ...
  
  private

  def policy_with_namespace(object_to_auth)
    controller_path.split('/').map(&:to_sym) << object_to_auth
  end
end

I'm pretty sure that would produce a plural for the name of the policy, but you could always call .singularize on the last item in the array.

You could then have this in a controller:

  def show
    authorize policy_with_namespace(@photo), :show?
    # ...
  end

I do agree that having something about namespacing in the docs would be useful, I had to look around to find the implementation we ended up using.

@wayne5540
Copy link
Author

Thanks @maxshelley, very helpful tip! I didn't know it's able to use namespace to authorize, I think it's a good approach because no method has been overrode. However, override policy(record) works great for me as well :)

And yes, my point is there should be some docs about namespace, I had to dig source code to find the implementation, and it took time... 😅

@edusantana
Copy link

edusantana commented Jan 15, 2018

@maxshelley how about this:

if you have an Admin namespace, you should have a AdminController for that namespace.

class Admin::AdminController < ApplicationController
end

class Admin::PhotosController < Admin::AdminController # instead of ApplicationController
end

Now, we have to know when we at are the admin path. We coulddo that from the controller. The ideia here is to add a dynamic method (at_admin?) to the instance with our context

class ApplicationController < ActionController::Base
  include Pundit

  def pundit_user
    def current_user.at_admin?
      false
    end
    current_user
  end

end

class Admin::AdminController < ApplicationController
  def pundit_user
    def current_user.at_admin?
      true # every subclass will have this context true
    end
    current_user
  end
end

Now, at the policy files i would be able to call user.at_admin? and know my context.

We can even have an organization:

class ApplicationController < ActionController::Base
  include Pundit

  before_action :get_organization

  def pundit_user
    def current_user.at_admin?
      false
    end
    # adding organization to context
    def current_user.current_org
      @org
    end
    current_user

  end

  private
  def get_organization
    @org = ...
  end
end

That's what i'm trying to do.

@maxshelley
Copy link

@edusantana I've read that a couple of times, but I'm not sure what you're trying to achieve with the context you're trying to add. If you need something in the policy, I suggest either passing it as part of the array in authorize, or using the additional context solution suggested in the readme.

Although I would bear this in mind (from the readme):

Pundit strongly encourages you to model your application in such a way that the only context you need for authorization is a user object and a domain model that you want to check authorization for. If you find yourself needing more context than that, consider whether you are authorizing the right domain model, maybe another domain model (or a wrapper around multiple domain models) can provide the context you need.

Pundit does not allow you to pass additional arguments to policies for precisely this reason.

Hope that's useful.

@edusantana
Copy link

@maxshelley the question was: how to authorize with namespaces? Supose we have two controllers (one without namespace, and the other with admin), and one model.

The pundit way is to have only one Policy file, since it only have one model.

When authorizing, how to know if we are at :admin of not? My solution was to use the controller to add a method at_admin to users.

@maxshelley
Copy link

The pundit way is to have only one Policy file, since it only have one model.

@edusantana I don't think this is the case, I think when it says “the only context you need for authorization is a user object and a domain model that you want to check authorization for” it means “only auth one model at a time”, not “you should only have one policy file”.

Using the Photo model from my earlier example, here's how we're working.

In the controllers, we add a namespace to the array that's passed to authorize.

class PhotosController < ApplicationController
  def show
    authorize [@photo], :show?
  end
end
class Admin::PhotosController < ApplicationController
  def show
    authorize [:admin_policy, @photo], :show?
  end
end

Note: we use :admin_policy rather than :admin as we had issues with namespaced policies clashing with namespaced models.

Then we end up with two policy classes. Note the different paths:

# app/policies/photo_policy.rb
class PhotoPolicy < ApplicationPolicy
  def show?
    # perform auth logic e.g. user.can_see_photo?
  end
end
# app/policies/admin_policy/photo_policy.rb
class Admin::PhotoPolicy < ApplicationPolicy
  def show?
    # perform auth logic e.g. user.admin?
  end
end

We're still only auth'ing one model at a time, we change the policy we're pointing towards based on the namespace. This seems to be working well for us so far.

@edusantana
Copy link

edusantana commented Feb 12, 2018

@maxshelley I see.

We will have to add this [:admin_policy, ...] to every controller action. If we have many models, and controllers, it will be at many places.

Instead of that, I suggest we could read from the controller's module name:

class PhotosController < ApplicationController
  def show
    authorize @photo
  end
end
class Admin::PhotosController < ApplicationController
  def show
    authorize @photo
  end
end

Then we end up with two policy classes:

# app/policies/photo_policy.rb
class PhotoPolicy < ApplicationPolicy
  def show?
    # perform auth logic e.g. user.can_see_photo?
  end
end
# app/policies/admin/photo_policy.rb
class Admin::PhotoPolicy < ApplicationPolicy
  def show?
    # perform auth logic e.g. user.admin?
  end
end

There's no need for this :admin_policy, we should read from the module name: Admin::...

Do you agree?

@maxshelley
Copy link

Instead of that, I suggest we could read from the controller's module name:
There's no need for this :admin_policy, we should read from the module name: Admin::...
Do you agree?

@edusantana I did reference this in the last part of my comment here: #497 (comment)

I don't think this needs to be in Pundit itself as you can implement this at your own end. Leaving it out allows us each to decide on our own implementation.

I much prefer the explicit nature of seeing the policy being called, the additional line in each action is well worth the trade-off for us.

@Linuus
Copy link
Collaborator

Linuus commented May 11, 2018

Thank you for the contribution!
I think the documentation about namespaces that I just merged is enough though:
https://github.com/varvet/pundit#policy-namespacing

@Linuus Linuus closed this May 11, 2018
@wayne5540 wayne5540 deleted the add-policy-override-readme branch May 12, 2018 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants