varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.26k stars 627 forks source link

'.authorize' and '#authorize' return record even with passed record with namespace array #626

Closed QWYNG closed 4 years ago

QWYNG commented 4 years ago

Hi, Thank you for the great Gem! This is a proposal and pull request at once. '.authorize' and '#authorize' return passed record now.

def show
  @post  = authorize Post.find(params[:id])
end

But When passing record with a namespace. return array

def show
  @post  = authorize [:admin, Post.find(params[:id])]
end

# @post is  [:admin, Post.find(params[:id])]

These methods are expected to return record not namespase array

When authorize override the helpers in AdminController to automatically apply the namespacing, This change will be very useful.

class AdminController < ApplicationController
  def authorize(record, query = nil)
    super([:admin, record], query)
  end
end

class Admin::PostController < AdminController
  def show
    post = authorize Post.find(params[:id])
    # We don't need `authorize(post)` line!
  end
end

If there is a place to fix, please let me know. best regards.

dgmstuart commented 4 years ago

While I think it's unlikely that anyone is relying on getting the namespace array back from the authorize method, technically this is a breaking change: I don't think there's a clear expectation set that we always return the model instance, since the README does say "authorize returns the object passed to it", which I'd argue could mean either "the instance" or "the array".

Not sure how seriously we need to take that 🤷‍♂

QWYNG commented 4 years ago

@dgmstuart Thank you for your comments! I'll fix spec! I think that this is a breaking change too. But I think that most users would expect the only record itself to return in this case.

QWYNG commented 4 years ago

@dgmstuart Thank you for your review. I fixed spec. I'm really grateful as a pundit user for your interaction even with this PR doesn't marge.

Linuus commented 4 years ago

I would definitely say this is a bug 🐛

QWYNG commented 4 years ago

@dgmstuart Thank you for your review! fixed again

QWYNG commented 4 years ago

@dgmstuart @Linuus I really appreciate both! Thank you for the useful gem again!