Forum: RSpec [rails] An authorization question

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
1b32ae81cae0813e933ca2f8be830c9f?d=identicon&s=25 Chris Flipse (Guest)
on 2009-02-28 19:02
(Received via mailing list)
I've been going back over some legacy code, backfilling tests, and I'm
encountering something that is causing no small amount of pain.  This is
in
a mature Rails app, that's  lived and migrated from 1.1 through to 2.1,
so
there's a lot of ancient cruft built up in the corners that I've been
trying
to clean up.

My question/pain point revolves around authorization.  In at least two
different models in the system  -- areas that are core to the
functionality
-- there are models that run through a state transition.  Only certain
users
are allowed to make those transitions, however.  You're basic "only an
admin
can publish an article" kind of restrictions.

These models show up across most of the app -- several different
controllers.  As such, long, long ago, someone patched updated the site
authentication code to assign a User.current singleton inside the
login_required filter.  This is then used by *several* models, sometimes
to
populate an updated_by stamp, sometimes it's actually used within a
models
validations(!), and it's definately used within some of the
state-transition
guards.

Now, this is really just a global variable by another name, and it's
pretty
well embedded after two years.  I've come upon a whole bunch of
different
pain points in trying to setup data (real data) within the cucumber
steps
I've been backfilling.  Lacking any support of injection, I end up doing
a
lot of juggling of the User.current value, just to get some test data
built
and in the right set of states ... and while I can bury the temporary
reassignments necessary inside a block, it still feels like it's an
intractable mess.

I know *why* this was originally done -- to avoid having to pass User
objects around all the time, and it does _appear_ to keep the API clean
--
but the hidden dependancy isn't really clean.

So, does anyone have any suggestions of how to easily manage model level
user authorization?
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2009-02-28 19:46
(Received via mailing list)
On Sat, Feb 28, 2009 at 11:52 AM, Chris Flipse <cflipse@gmail.com>
wrote:
> can publish an article" kind of restrictions.
>
> These models show up across most of the app -- several different
> controllers.  As such, long, long ago, someone patched updated the site
> authentication code to assign a User.current singleton inside the
> login_required filter.

Unless I'm missing something, this seems like the problem is wider
than testability.

Let's say I log in. Right now I am User.current. Now you log in, and
become User.current. Now I got to view some resource that I am not
permitted to see, but I get to see it because you are permitted and
YOU are the User.current.

Am I missing something?
1b32ae81cae0813e933ca2f8be830c9f?d=identicon&s=25 Chris Flipse (Guest)
on 2009-02-28 20:04
(Received via mailing list)
On Sat, Feb 28, 2009 at 1:38 PM, David Chelimsky
<dchelimsky@gmail.com>wrote:

> > My question/pain point revolves around authorization.  In at least two
> > authentication code to assign a User.current singleton inside the
> Am I missing something?
The login filter is called at the beginning of every request, from
application controller.  It resets the value, nilling it out if you're
not
logged in, at the start of each request.  So long as Rails remains
single
threaded, the scenario you describe isn't possible.  One process, one
request at a time.  No bleed there.

Of course, they're supposedly working on making it not-so single
threaded,
so that may eventually become a problem.  All the more reason to find
something that doesn't suck.
5d38ab152e1e3e219512a9859fcd93af?d=identicon&s=25 David Chelimsky (Guest)
on 2009-02-28 20:34
(Received via mailing list)
On Sat, Feb 28, 2009 at 12:51 PM, Chris Flipse <cflipse@gmail.com>
wrote:
>> > so
>> > admin
>> Let's say I log in. Right now I am User.current. Now you log in, and
> request at a time.  No bleed there.
>
> Of course, they're supposedly working on making it not-so single threaded,
> so that may eventually become a problem.  All the more reason to find
> something that doesn't suck.

:)

>> > different
>> > pain points in trying to setup data (real data) within the cucumber
>> > steps
>> > I've been backfilling.  Lacking any support of injection, I end up doing
>> > a
>> > lot of juggling of the User.current value

You can stub this in your code examples:

  User.stub!(:current).and_return(mock_model(User, :authorized? =>
true))

or

  User.stub!(:current).and_return(mock_model(User, :authorized? =>
false))

etc.

Replace "authorized? => true/false" with whatever is necessary for the
authorization to pass or fail as needed in each example.

Stubs are cleared out after each example, so you don't have to use any
additional injection techniques.

HTH,
David

>> >
>> > So, does anyone have any suggestions of how to easily manage model level
>> > user authorization?
1b32ae81cae0813e933ca2f8be830c9f?d=identicon&s=25 Chris Flipse (Guest)
on 2009-02-28 21:14
(Received via mailing list)
I've actually been okay with it at the unit testing / rspec level --
I've
had it stubbed as you describe for a while.

The pain point came in as I was trying to setup data for Cucumber ...
Which,
listening to my tests, tells me that the current structure is bad.  I
was
more curious to see how others are handling that sort of situation.

I want to get *away* from the global variable, I'm just not entirely
sure
what the target should be.  There doesn't seem to be a whole lot of talk
about actual implementation specifics around model level authorization.
C694a032be7518a0d704318895f8fe1d?d=identicon&s=25 Ben Mabey (mabes)
on 2009-02-28 22:02
(Received via mailing list)
Chris Flipse wrote:
> I've actually been okay with it at the unit testing / rspec level --
> I've had it stubbed as you describe for a while.
>
> The pain point came in as I was trying to setup data for Cucumber ...
> Which, listening to my tests, tells me that the current structure is
> bad.  I was more curious to see how others are handling that sort of
> situation.

If you are seeing state from one scenario bleed over to the next I would
suggest something like this in your env.rb:

After do
 User.reset_current
end
>
> I want to get *away* from the global variable, I'm just not entirely
> sure what the target should be.  There doesn't seem to be a whole lot
> of talk about actual implementation specifics around model level
> authorization.

I generally have a current_user method defined of my controller to
return the logged in user.  Assuming that your app is only using
User.current in your controllers you could try to move towards something
like that... If you have models accessing User.current then it truly is
being used as a global. :/  The user will then have some permissions
methods that may take other objects or symbols.  The method will simply
return a boolean telling if the user is authorized or not.  That logic
usually is based on the role(s) of that user or relationship with the
passed in object.  Having this logic in the user could be viewed as a
responsibility issue- should the user really be responsible for telling
if it is authorized for everything?  In general I do this for most
simple cases.  Only when it starts to get complex do I move it out into
a Manager-like object.

HTH,
Ben
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-02-28 22:45
(Received via mailing list)
On Sat, Feb 28, 2009 at 2:54 PM, Chris Flipse <cflipse@gmail.com> wrote:
> I've actually been okay with it at the unit testing / rspec level -- I've
> had it stubbed as you describe for a while.
>
> The pain point came in as I was trying to setup data for Cucumber ... Which,
> listening to my tests, tells me that the current structure is bad.  I was
> more curious to see how others are handling that sort of situation.
>
> I want to get *away* from the global variable, I'm just not entirely sure
> what the target should be.  There doesn't seem to be a whole lot of talk
> about actual implementation specifics around model level authorization.

Disclaimer, this entire post has to deal with model level
authorization and a technique I've been using and evolving in Rails
projects. It has nothing to do with your issue of global state for
what User is logged in. Onto the fun stuff IMO....

I've been wanting to write a thorough blog post on thoughts and
reflections on a very related topic, but since you've already kicked
off the conservation I'll try to add to the discussion. :)

For nearly a year I've been utilizing the concepts of Roles and also
Privileges. They are close at heart, but different needs have proven
both of them to be successful at different times.

Here's an example of being reliant on a role:

  # raises if the user cannot fulfill the role
  supervisor = user.in_role(:supervisor)
  supervisor.do_supervisory_thing

And the supervisor role looks like:

  class Roles::Supervisor < Roles::Base
    def do_supervisory_thing
      # inside the implementation you
      # can refer to the source of the role
      # using the method #source
    end
  end

The privilege functionality is similar:

  # this will raise access denied if user doesn't have the privilege
  payer = user.with_privilege(:make_payment)
  payer.pay!(invoice)

And its implementation looks like:

  class Privileges::MakePayment < Privileges::Base
    def pay!(invoice)
      Payment.create! :invoice => invoice
    end
  end

This abstracts out the behaviour surrounding privileges and roles out
into their own component. In most cases the methods are very small,
and they merely create real models, or do other things. The big win
for me has been that they create a concrete representation of
something tied to a role or a privilege. I don't know how much more
intention-revealing I can get. :)

Right now I am leaving things a little verbose in the controller
actions, e.g. user.in_role(...) or user.with_privilege(...), but I
like it because no one has to guess what is being used. The less
verbose route would be to omit #in_role or #with_privilege and just
say:

  user.pay!(invoice)

The user would be in charge of searching its roles/privileges for one
that responded to #pay!. I am considering moving to this route, there
are some edge cases that would need to be solved.

If you're interested, the roles project is on github:
http://github.com/zdennis/roles/tree/master

I just through up a wiki page for using them with Rails if you want to
test it out: http://wiki.github.com/zdennis/roles/rails





>> > wrote:
>> >> > so
>> >> > users
>> >>
>> > The login filter is called at the beginning of every request, from
>> > something that doesn't suck.
>> >> > Now, this is really just a global variable by another name, and it's
>> You can stub this in your code examples:
>> authorization to pass or fail as needed in each example.
>> >> > reassignments necessary inside a block, it still feels like it's an
>> >> > user authorization?
>>
>>
>>
>>



--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
1b32ae81cae0813e933ca2f8be830c9f?d=identicon&s=25 Chris Flipse (Guest)
on 2009-03-01 00:02
(Received via mailing list)
On Sat, Feb 28, 2009 at 4:34 PM, Zach Dennis <zach.dennis@gmail.com>
wrote:

>
> Disclaimer, this entire post has to deal with model level
> authorization and a technique I've been using and evolving in Rails
> projects. It has nothing to do with your issue of global state for
> what User is logged in. Onto the fun stuff IMO....


It's actually a lot closer to what I'm trying to get to the bottom of
than
the global variable bit is.


> Here's an example of being reliant on a role:
>
>  # raises if the user cannot fulfill the role
>  supervisor = user.in_role(:supervisor)
>  supervisor.do_supervisory_thing
>

Okay.  There actually is something fairly close to this.

Take a group blog with an editor/supervisor.  Anyone can write up and
draft
a document, but only the supervisor can publish it.

Right now, I do have a User#is_supervisor_of?(group) method.  Good
enough
there.

Problem is, where does that check _happen_ ?  At the model level?  Or in
the
controller?

You can externalize the request in the controller:

  if current_user.is_supervisor_of?( document.group)
    document.published_by = current_user
    document.publish!
  else
    # flag some errors
  end

But if it's the sort of action that finds itself in a couple of
different
controllers, there's been a worry about repetition -- forgetting logic
and
such.  Which is why the code is really more like this:

  # in the model
  def publish!
    if User.current.is_supervisor_of?(group)
      self.published_by = User.current
      self.state = 'published'
    else
      raise NotASupervisorError
    end
  end

Only it's actually a bit worse than that, because I think it's wormed
it's
way into the validations...

No controller can forget to check the authorization.  However, it makes
setting up test data -- and even working from the console -- painful.
Among
other things, there a business rule in place that you can't approve your
own
document.


>
>      Payment.create! :invoice => invoice
> Right now I am leaving things a little verbose in the controller
>
> If you're interested, the roles project is on github:
> http://github.com/zdennis/roles/tree/master
>
> I just through up a wiki page for using them with Rails if you want to
> test it out: http://wiki.github.com/zdennis/roles/rails
>

I'll give it a read, see what I can learn
1b32ae81cae0813e933ca2f8be830c9f?d=identicon&s=25 Chris Flipse (Guest)
on 2009-03-01 00:03
(Received via mailing list)
On Sat, Feb 28, 2009 at 3:42 PM, Ben Mabey <ben@benmabey.com> wrote:

> If you are seeing state from one scenario bleed over to the next I would
> suggest something like this in your env.rb:
>
> After do
> User.reset_current
> end
>

Yep got that.  The tests are actually *working*, it's just that the
setup
has gotten painful.


>
>> I want to get *away* from the global variable, I'm just not entirely sure
>> what the target should be.  There doesn't seem to be a whole lot of talk
>> about actual implementation specifics around model level authorization.
>>
>
> I generally have a current_user method defined of my controller to return
> the logged in user.


Same.  Authentication was originally generated using
acts_as_authenticated,
so the standard conventions at the controller level are in place.


> Assuming that your app is only using User.current in your controllers you
> could try to move towards something like that... If you have models
> accessing User.current then it truly is being used as a global. :/


It truly is being used as a global.


> The user will then have some permissions methods that may take other
> objects or symbols.  The method will simply return a boolean telling if the
> user is authorized or not.  That logic usually is based on the role(s) of
> that user or relationship with the passed in object.  Having this logic in
> the user could be viewed as a responsibility issue- should the user really
> be responsible for telling if it is authorized for everything?  In general I
> do this for most simple cases.  Only when it starts to get complex do I move
> it out into a Manager-like object.
>

Yes!  This is what I was trying (poorly) to get at.

Responsibility issues might be a large part of why it got factored this
way
to begin with.  The global is bad.  Really bad, which is why I'm trying
to
figure out something that works better.  But I believe it was put in
place
so that a model can be responsible for it's own authorization.  Some of
the
models are used and updated from several different controllers, so any
authorization logic external to the model would have had to be repeated
in
several different locations.

The concern with that might be an over-enthusiastic embrace of DRY.
However
some of the authorization stuff is Really Really Important, so embedding
the
authorization logic in the model itself was seen as a way to ensure it's
not
forgotten about.

Half of my problem right now is that I'm not even sure what *layer* to
put
model specific authentication!  If it's in the controller layer, it's
repeated logic in every controller that touches the model in question.
If
it's in the model, the logic is centralized, but now your model needs
not
only to know about Users in general, it needs a specific user.  You have
less chance of someone doing Something They Shouldn't due to a forgotten
check in a controller, but the test setup seems to suffer for it.

One way or the other, the global User.current is going away -- soon.
It's
just a question of what to replace it with, and where.
D2e1afa2b21c86359cef59d6d0adc790?d=identicon&s=25 Andrew Henson (Guest)
on 2009-03-01 22:15
(Received via mailing list)
On 28 Feb 2009, at 22:26, Chris Flipse wrote:
> different locations.
> your model needs not only to know about Users in general, it needs a
> specific user.  You have less chance of someone doing Something They
> Shouldn't due to a forgotten check in a controller, but the test
> setup seems to suffer for it.
>
> One way or the other, the global User.current is going away --
> soon.  It's just a question of what to replace it with, and where.

I was only skim-reading this topic so I may be misunderstanding what
you're after but I think that maybe what you're looking for is
something like
http://github.com/stffn/declarative_authorization/...
, a Rails plugin that allows you to specify the authorisation in a
single place for both controllers and at the model level. I've just
started using it for a project and so far it seems a good fit, though
I'm trying to keep the whole app as restful resources which makes
things a little easier.

It also has a few test helper methods which make it really easy to use
with Cucumber and RSpec.

Andy
85d99e7678d8720f6e00ab0f60fe6ea9?d=identicon&s=25 Andrew Premdas (Guest)
on 2009-03-02 15:58
(Received via mailing list)
Personally I think this should be in the model layer. As you've pointed
it
you get alot of repetition if its in the controller layer. But putting
business logic in the controller layer goes against the fundamentals of
MVC,
and is a bit of a Rails anti-pattern (fat controller).

So given that the rules should be in the model then the question is
which
part of the model should have this responsibility and how do you call
it.
Three choices come to mind here

1) Place the rule inside User
2) Place the rule inside the affected model
3) Create a new model(s) to encapsulate this functionality. It might be
helpful to think of this as a service which models can use

1) Pollutes User with additional responsibility, but you can live with
this
so long as things remain very simple

2) Isn't DRY if the rules apply to many different models

3) Is more complex

It sounds like you have 1) and should consider moving to 3). However you
might get some change out of improving 1) first if you can, by using
standard refactorings

Have you considered creating a spike and doing a bit of BDD on the
spike.
i.e write the stories you want to write, implement simple code for them
and
try and work out the interactions you need from there. Even if you don't
use
the spike code in your application this might help you get a better set
of
features for this piece of functionality, (not tainted by the
implementation
of the existing code. These features then can be applied to your legacy
project and perhaps other projects as well.

HTH

Andrew


2009/2/28 Chris Flipse <cflipse@gmail.com>
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-02 19:39
Andrew Premdas wrote:

> So given that the rules should be in the model then the question is
> which part of the model should have this responsibility and how do
> you call  it.
> Three choices come to mind here
>
> 1) Place the rule inside User
> 2) Place the rule inside the affected model
> 3) Create a new model(s) to encapsulate this functionality. It might be
> helpful to think of this as a service which models can use
>
> 1) Pollutes User with additional responsibility, but you can live with
> this so long as things remain very simple
>

I am not sure that this is really "pollution".  One of the things that
was pointed out to me on the Ruby list when I first began transitioning
to OO was the mantra "ask" don't "tell".  It seems to me that in an OO
authorization scheme one might properly ask the user instance (model)
whether or not they are permitted to do "something" (controller) rather
than have the "something" test to see if that user is permitted.
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-02 19:48
James Byrne wrote:

>
> I am not sure that this is really "pollution".  One of the things that
> was pointed out to me on the Ruby list when I first began transitioning
> to OO was the mantra "ask" don't "tell".  It seems to me that in an OO
> authorization scheme one might properly ask the user instance (model)
> whether or not they are permitted to do "something" (controller) rather
> than have the "something" test to see if that user is permitted.

Unless I have misunderstood your intent and by your third choice you are
referring to an external role based model while your first choice refers
to putting the actual rules inside the user model.  In which case I
agree with you.

My comments refer to the idea that the user model makes the calls to the
role model and returns whether or not they were authorized to the
request.
48641c4be1fbe167929fb16c9fd94990?d=identicon&s=25 Mark Wilden (Guest)
on 2009-03-02 20:12
(Received via mailing list)
On Mon, Mar 2, 2009 at 10:39 AM, James Byrne <lists@ruby-forum.com>
wrote:
>
> I am not sure that this is really "pollution".  One of the things that
> was pointed out to me on the Ruby list when I first began transitioning
> to OO was the mantra "ask" don't "tell".

Actually, it's the other way around.

http://www.pragprog.com/articles/tell-dont-ask

///ark
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-02 20:23
Mark Wilden wrote:
>
>
> Actually, it's the other way around.
>
> http://www.pragprog.com/articles/tell-dont-ask

Sigh...
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-02 20:37
Mark Wilden wrote:

>
> Actually, it's the other way around.
>
> http://www.pragprog.com/articles/tell-dont-ask
>

I have read this article and it leaves me rather more confused than not.
I gather that I am missing something fundamental.  Consider that when I
write x.to_s I am telling the object what to give back.  But then what
am I doing when I write x.exists? or x.is_a?(y) or
sam.authorized?(controller_or_model, action)?

Is this a semantic confusion on my part?  Should I consider that what I
do with x.exists? is tell the object to answer a question?
Aafa8848c4b764f080b1b31a51eab73d?d=identicon&s=25 Phlip (Guest)
on 2009-03-02 21:13
(Received via mailing list)
James Byrne wrote:

> Is this a semantic confusion on my part?  Should I consider that what I
> do with x.exists? is tell the object to answer a question?

Consider...

   if x.exists?
     x.important_method()
   else
     # nothing!
   end

Now lets upgrade the variable x. Sometimes it points to a thing that
exists, and
sometimes it points to a stub object whose only job is to return 'false'
from
exists?, and do nothing inside important_method():

   if x.exists?
     x.important_method()
   else
     x.important_method()
   end

Now you can take out the if!

   x.important_method()

We are no longer asking x if it exists. We are telling it what to do,
and
letting it decide how to do it. That is the heart of OO programming.

(There is still an 'if', somewhere - the one that populates x. But -
hopefully -
it's only in one place.)

--
   Phlip
   http://www.zeroplayer.com/
942a74b1b71ca62234ae690b44699ebc?d=identicon&s=25 unknown (Guest)
on 2009-03-02 21:32
(Received via mailing list)
----- Original Message ----
> From: James Byrne <lists@ruby-forum.com>

> Mark Wilden wrote:
> > Actually, it's the other way around.
> >
> > http://www.pragprog.com/articles/tell-dont-ask
>
> I have read this article and it leaves me rather more confused than not.

That's the danger of oversimplification.  Another way to phrase it is,
don't rely on objects for things they know.  Rely on them for things
they know how to do.

Now it boils down to who's responsible.  The door's lock is responsible
for reading the key, and the bolt is responsible for unlocking the door.
The door is only responsible for letting me in, along with some cold
air.  The key is a role here, the lock is the controller, and the
bolt... okay, the analogy breaks down again.  But consider zones of
responibility.

> sam.authorized?(controller_or_model, action)?

I'll suggest that it's the controllers who are responsible for telling
what role or other requirements need to be satisfied to get their
services, and that it's the job of the user object (maybe by delegating
to some role class or objects) to provide the information as to whether
those requirements are met.

I smell something when I think about individual models specifying their
requirements.  Front-gate access through the controller actions smells
more correct to me.

If your actions can't be boiled down that atomically, I ask the
question, "is there something else wrong?".

Randy
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-02 21:59
unknown wrote:

>
>> sam.authorized?(controller_or_model, action)?
>
> I'll suggest that it's the controllers who are responsible for telling
> what role or other requirements need to be satisfied to get their
> services, and that it's the job of the user object (maybe by delegating
> to some role class or objects) to provide the information as to whether
> those requirements are met.
>


That is what I thought that I was doing.  The Controller sends the
message to the User Instance telling it to answer the question: are you
authorized to perform "controller + action"?; or role, or whatever the
controller sends as the criteria to be met.  It seems to me necessary
that the User model receive the context of the authorization call.  Now
the actual check on whether user x is authorized to perform the create
method of the PaymentReceivedController is done in the #authorized?
method of User.

Is this what should be done or is there a different way?
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-03-02 23:39
(Received via mailing list)
On Mon, Mar 2, 2009 at 3:59 PM, James Byrne <lists@ruby-forum.com>
wrote:
>>
>
> Is this what should be done or is there a different way?

Today, my pair and I hit a scenario which I think maybe a good example
to clarify.

Let's say that you have an "admin" and "associate" role. Each role can
access invoices in the system, but each role can access different
subsets of invoices.

In our controller, we could have something like:

def index
   if user.has_role?("admin")
      Invoice.all
   elsif user.has_role?("associate")
      Invoice.by_area(current_user.area)
   else
     raise AccessDenied
   end
end

We ended up not doing this because the lines "Invoice.all" and
"Invoice.by_area" is behaviour which is tied specifically to a certain
role in the system. We don't want behaviour for these roles to be
scattered throughout controller actions. We want it in one single
location.

So we create a FiscalAdmin role object and a FiscalAssociate role
object. As these roles become filled out we leave the role-specific
behaviour in one location, creating a well-defined cohesive class for
each role.

For me it's a little win, that results in big ways. :)


--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-03-02 23:47
(Received via mailing list)
On Mon, Mar 2, 2009 at 4:50 PM, Zach Dennis <zach.dennis@gmail.com>
wrote:
>>> those requirements are met.
>> method of User.
> In our controller, we could have something like:
>
>
> For me it's a little win, that results in big ways. :)
>

Forgot to mention what we did do. We ended up with the following...

def index
  if user.has_role?("admin")
    user.in_role("admin").invoices
  elsif user.has_role?("associate")
    user.in_role("associate").invoices
  else
    raise AccessDenied
  end
end

To us, the change here is subtle, but important. The controller is
allowed to ask for invoices from each role, but is not allowed to know
how find the invoices, that's the behaviour of the role.

> --
> - Show quoted text -
> Zach Dennis
> http://www.continuousthinking.com
> http://www.mutuallyhuman.com
>



--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
994e42bda994be2cd1d791f18ee6d561?d=identicon&s=25 Stephen Eley (Guest)
on 2009-03-03 05:45
(Received via mailing list)
On Mon, Mar 2, 2009 at 5:16 PM, Zach Dennis <zach.dennis@gmail.com>
wrote:
>  end
> end

That seems sort of backwards to me.  These aren't the user's invoices,
right?  They're just invoices which the user happens to be allowed to
see?  Chaining it this way makes it look like the invoices *belong* to
the role, and seems put the user up front instead of the invoices.
You also have conditional branching with hardcoded values, making the
controller brittle, and some redundancy with the controller asking the
model for a value and then passing the value right back to the model.

Can a model have more than one role?  If it can, you have a problem
here because the 'if' will only ever *act* on one role.  If it can't,
life gets simple:

[controller]
def index
  @invoices = Invoice.by_role(user)
rescue AccessDenied
  flash[:warning] = "Nope.  Sorry.  Nice try."
  redirect_to :back
end

[Invoice model]
def by_role(user)
  case user.role
  when "admin"
    [whatever]
  when "associate"
    [whatever]
  else
    raise AccessDenied
  end
end

...That could probably still be made more elegant.  I'm not a huge fan
of case statements, and I have in my head some idea that you could
have named scopes for each role and use "send" to pick the right
scope.  But that could be overdesigning it, and in any case I don't
really know what each role has to return in your business case.

The important takeaway here is that the Invoice is responsible for
figuring out what to return, based on user data passed to it at
runtime; the User doesn't have to have special logic to know how to
query every other model in the system.


--
Have Fun,
   Steve Eley (sfeley@gmail.com)
   ESCAPE POD - The Science Fiction Podcast Magazine
   http://www.escapepod.org
994e42bda994be2cd1d791f18ee6d561?d=identicon&s=25 Stephen Eley (Guest)
on 2009-03-03 06:22
(Received via mailing list)
On Sat, Feb 28, 2009 at 5:26 PM, Chris Flipse <cflipse@gmail.com> wrote:
>
> Half of my problem right now is that I'm not even sure what layer to put
> model specific authentication!  If it's in the controller layer, it's
> repeated logic in every controller that touches the model in question.  If
> it's in the model, the logic is centralized, but now your model needs not
> only to know about Users in general, it needs a specific user.

My two cents:

1.) I feel authorization belongs in two places: models and views.
Models need to know what they're allowed to do.  Authorization becomes
a scope on reads and a validation on updates.  Views (specifically
helper methods) need to know whether they're allowed to show that
"Edit" button, etc.  That's not critical path, that's navigation,
maybe one step up from cosmetics.  I don't see a reason why
controllers need to know as long as they can handle nils coming back
from the models.

2.) In both cases, you need to know who the current user is, and
that's fine.  Figuring it out is the job of *authentication*, not
authorization.  Your authentication stack just needs to give you a
hook where you can ask it for the user, and your authorization stack
should handle it sensibly if the answer is 'nil.'  The
restful_authentication plugin implements current_user as a global
Application method, but that's not the only way to do it.

3.) Consider separating the authorization stuff from the core business
logic of your app, and implementing it as a module on the authorizable
classes instead.  Then if your basic authz behavior changes, you
(ideally) only have to change it in one place.  And it doesn't mess up
the readability of your main behavior.

...And because everyone else is doing it, here are my design notes on
my own overcomplicated authorization system (which, caveat, has yet to
be built, but I wrote this all down anyway to get it out of my head):

http://extraneous.org/past/2009/2/6/mother_do_you_...

I know it looks rather overdone, and it's perfectly possible that it
is, but my requirements were sincere:

* it needs to handle both users and groups, and
* I wanted it to be hierarchical, such that privileges on parent pages
trickle down to their children.

Between those two constraints, I couldn't off the top of my head think
of anything more elegant.



--
Have Fun,
   Steve Eley (sfeley@gmail.com)
   ESCAPE POD - The Science Fiction Podcast Magazine
   http://www.escapepod.org
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-03-03 06:30
(Received via mailing list)
On Mon, Mar 2, 2009 at 11:35 PM, Stephen Eley <sfeley@gmail.com> wrote:
>>    raise AccessDenied
>>  end
>> end
>
> That seems sort of backwards to me.  These aren't the user's invoices,
> right?  They're just invoices which the user happens to be allowed to
> see?  Chaining it this way makes it look like the invoices *belong* to
> the role, and seems put the user up front instead of the invoices.

I agree, it could be better.

> You also have conditional branching with hardcoded values, making the
> controller brittle, and some redundancy with the controller asking the
> model for a value and then passing the value right back to the model.

I would like to push down the conditional logic to a lower part of the
app and there probably is a way that I haven't found yet with the
technique I'm using. Right now I'm exploring some trade-offs. Put more
emphasis on the responsibility of a role or on the responsibility of
the model? I've been down the model route, and would like to see where
the role route takes me. :)


>  redirect_to :back
>    raise AccessDenied
>  end
> end
>
> ...That could probably still be made more elegant.  I'm not a huge fan
> of case statements, and I have in my head some idea that you could
> have named scopes for each role and use "send" to pick the right
> scope.  But that could be overdesigning it, and in any case I don't
> really know what each role has to return in your business case.

I've been down this path many times. It has worked well when the
privilege/role set was limited and fairly simple, but seems to leave
models convoluted for anything else. That's what sparked me to explore
concretely identifying the Role in my app, and allowing it to make
decisions that are unique to it, rather than sprinkling them
throughout the models.

>
> The important takeaway here is that the Invoice is responsible for
> figuring out what to return, based on user data passed to it at
> runtime; the User doesn't have to have special logic to know how to
> query every other model in the system.

To clarify, the User doesn't know how-to query anything. All it knows
is if it can fulfill a Role. If it can it returns the appropriate role
object. Each role is responsible for knowing what it can access, but
it doesn't do the nitty gritty work. It calls methods on other models.
For the Invoice example, the associate role calls
Invoice.by_area(user.area), whereas the Admin role calls Invoice.all
in each of their respective #invoices methods.

Some of what has piqued my interest in exploring apps that place more
emphasis on Roles and Privileges is that it it's difficult to
understand what it means to be an admin when what it means to be able
to to be an "admin" or  "supervisor" or a "team supervisor" or an
"employee" is sprinkled throughout the app. So far I have enjoyed
having the responsibility of an admin in one spot, even though that
Admin object doesn't deal with the details. It just knows how-to make
a decision and then it tells another object to do it. It's kind of
like inserting a thin-layer of roles and privileges between the
application layer and the domain layer. I'd say these sit on the top
end of the domain layer.

As you pointed out earlier, the API for #in_role could use a little
love.

--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
48641c4be1fbe167929fb16c9fd94990?d=identicon&s=25 Mark Wilden (Guest)
on 2009-03-03 07:15
(Received via mailing list)
On Mon, Mar 2, 2009 at 8:35 PM, Stephen Eley <sfeley@gmail.com> wrote:

>  @invoices = Invoice.by_role(user)

It doesn't seem right to me that invoices know about users and roles.
I think of invoices are being closer to the metal -- closer to the
essence of the application -- than petty concerns like authorization.

I would try something like

 user.role.invoices

where Role is a model that does the traffic-cop work of deciding what
invoices are available to it.

///ark
994e42bda994be2cd1d791f18ee6d561?d=identicon&s=25 Stephen Eley (Guest)
on 2009-03-03 07:36
(Received via mailing list)
On Tue, Mar 3, 2009 at 1:04 AM, Mark Wilden <mark@mwilden.com> wrote:
> On Mon, Mar 2, 2009 at 8:35 PM, Stephen Eley <sfeley@gmail.com> wrote:
>
>>  @invoices = Invoice.by_role(user)
>
> It doesn't seem right to me that invoices know about users and roles.
>
> I would try something like
>  user.role.invoices

Heh.  Which is what Zach said he wanted to do, and it isn't wrong.
But it doesn't seem right to *me* that roles know about invoices.
>8->  As I see it, if you go that path you end up having to inform
roles about every *other* model, and consolidating all your business
logic in one class.  At which point you're risking drifting away from
object-oriented programming and heading back to big procedures and
spaghetti code.

But that's a kneejerk response, and it needn't be that bad.  It really
comes down to taste.  To-mae-to, to-mah-to.  Sooner or later it's all
ones and zeros.


--
Have Fun,
   Steve Eley (sfeley@gmail.com)
   ESCAPE POD - The Science Fiction Podcast Magazine
   http://www.escapepod.org
48641c4be1fbe167929fb16c9fd94990?d=identicon&s=25 Mark Wilden (Guest)
on 2009-03-03 09:23
(Received via mailing list)
On Mon, Mar 2, 2009 at 10:34 PM, Stephen Eley <sfeley@gmail.com> wrote:
> On Tue, Mar 3, 2009 at 1:04 AM, Mark Wilden <mark@mwilden.com> wrote:

>>  user.role.invoices
>
> Heh.  Which is what Zach said he wanted to do, and it isn't wrong.

Actually, I thought Zach was talking about a method on User called
in_role.

> But it doesn't seem right to *me* that roles know about invoices.

Roles know about access to invoices. That's what their purpose is - to
let people do some things and not let them do others.

>>8->  As I see it, if you go that path you end up having to inform
> roles about every *other* model, and consolidating all your business
> logic in one class.

You do consolidate all your _access_ logic in one class, just as you
might consolidate all your sales tax knowledge in another class. That
way you have one source of responsibility for that behavior.
Otherwise, if you added, changed or deleted a role, you'd have to
change every model.

This is basically the Mediator pattern. Pluses and minuses, to be sure.

///ark
171ea139761951336b844e708d1547ab?d=identicon&s=25 James Byrne (byrnejb)
on 2009-03-03 16:08
Mark Wilden wrote:

>
> You do consolidate all your _access_ logic in one class, just as you
> might consolidate all your sales tax knowledge in another class. That
> way you have one source of responsibility for that behavior.
> Otherwise, if you added, changed or deleted a role, you'd have to
> change every model.
>
> This is basically the Mediator pattern. Pluses and minuses, to be sure.
>

This is not where I am spending my time at the moment, but it is an area
that i am going to confront sooner than later.  So, I am interested in
this subject.

If I understand aright, it is agreed that authorization is best
determined by the user model, either as a full-blown authorization
method wholly contained in the User class or as a call to a subordinate
Authorization class which does the heavy lifting and then returns the
result to the User class.  The controllers (and perhaps models) that
require authorization simply call the equivalent to an "authorized?"
method on the current_user passing the context, whether this be a named
role or a control/action pair or a model/attribute pair.  The
User.authorized? method simply replies whether this current_user belongs
to the named role or otherwise has the privilege of performing the
desired action.

If there is an Authorization class then this is where the work is done
(lookups on models or tables as the case may be).

Does this capture the essence?

The question that I have is: If one implements an authorization system
as a model or models then would this best be implemented in the user as
an association rather than as a separate Authorization class?
85d99e7678d8720f6e00ab0f60fe6ea9?d=identicon&s=25 Andrew Premdas (Guest)
on 2009-03-03 17:14
(Received via mailing list)
I think this discussion has gone backwards a bit. Here is what I think
the
index method in the invoices controller should be like

def index
  begin
   invoice.get_collection
  rescue
    # decide what to do if we can't get collection
  end
end

Now clearly this needs some work to get it to work ...

1) What is 'invoice'

Rails by default ties 'invoice' to a class in app/model. Usually this
ActiveRecord model class, but it doesn't have to be. We can always put
another layer inbetween (e.g. Presenter) if it makes our code simpler

2) Authentication parameters

Clearly these need to be passed through to get_collection. This can be
done
by parameters or by making the authentication available in a wider
context.

3) Exceptions

We need an exception hierarchy. NotAuthorised, NotFound etc.

All the controller should do is get the collection and deal with
exceptions
if the collection is not available. (n.b. the collection being empty is
not
exceptional.)

Rails historically has corrupted (compromised, polluted ...) MVC by
allowing
concerns of how we get the collection to be included in the controller.
RESTful design has highlighted the problems with this and now we end up
with
this situation where things like authentication and authorisation don't
really have an obvious place.

These things - authentication, authorisation and the exception handling
(for
the resource) - are services which all resources need access to. They
need
to be seperated and applied in a cross-cutting manner. Perhaps we could
do
things more elegantly with an Aspect Orientated solution.

Andrew



2009/3/2 Zach Dennis <zach.dennis@gmail.com>
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-03-03 18:18
(Received via mailing list)
On Tue, Mar 3, 2009 at 11:07 AM, Andrew Premdas <apremdas@gmail.com>
wrote:
>
> Now clearly this needs some work to get it to work ...
>
> 1) What is 'invoice'
>
> Rails by default ties 'invoice' to a class in app/model. Usually this
> ActiveRecord model class, but it doesn't have to be. We can always put
> another layer inbetween (e.g. Presenter) if it makes our code simpler

Presenters are for presentation logic, not for authorization concerns.

>
> 2) Authentication parameters
>
> Clearly these need to be passed through to get_collection. This can be done
> by parameters or by making the authentication available in a wider context.

Authentication isn't the issue here, authorization is.

> concerns of how we get the collection to be included in the controller.
> RESTful design has highlighted the problems with this and now we end up with
> this situation where things like authentication and authorisation don't
> really have an obvious place.

It is up to the controllers to know how-to ask for something, it
should not know how the internals of that request work. For a
controller to know what role can access a particular resource is
completely aligned with a layered architecture, keeping application
concerns in the right layer, and domain logic in another layer.

>
> These things - authentication, authorisation and the exception handling (for
> the resource) - are services which all resources need access to. They need
> to be seperated and applied in a cross-cutting manner. Perhaps we could do
> things more elegantly with an Aspect Orientated solution.
>

Is some cases yes, but I fail to see where it benefits or adds more
value then concretely identifying a role that has particular behavior.

>>  else
>> > Zach Dennis
>> > http://www.continuousthinking.com
>> > http://www.mutuallyhuman.com
>> >
>>
>>
>
>



--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
39100495c9937c39b2e0c704444e1b4a?d=identicon&s=25 Pat Maddox (Guest)
on 2009-03-03 18:46
(Received via mailing list)
On Mon, Mar 2, 2009 at 8:35 PM, Stephen Eley <sfeley@gmail.com> wrote:
>>    raise AccessDenied
>>  end
>> end
>
> That seems sort of backwards to me.  These aren't the user's invoices,
> right?  They're just invoices which the user happens to be allowed to
> see?  Chaining it this way makes it look like the invoices *belong* to
> the role, and seems put the user up front instead of the invoices.
> You also have conditional branching with hardcoded values, making the
> controller brittle, and some redundancy with the controller asking the
> model for a value and then passing the value right back to the model.

Agreed.  I have a similar example in a blog post:
http://www.patmaddox.com/blog/2008/7/23/when-dupli...

Pat
F86901feca747abbb5c6c020362ef2e7?d=identicon&s=25 Zach Dennis (zdennis)
on 2009-03-03 19:19
(Received via mailing list)
On Tue, Mar 3, 2009 at 12:32 PM, Pat Maddox <pat.maddox@gmail.com>
wrote:
>>>  else
>> model for a value and then passing the value right back to the model.
>
> Agreed.  I have a similar example in a blog post:
> http://www.patmaddox.com/blog/2008/7/23/when-dupli...

I agree I'm taking a step back to try to make two steps forward. The
heart of the exploration is more than the conditional in the action
(which simply states which roles are allowed to access that action, I
just made it explicit rather than using a #restrict_to call). To me
this discussion is about where the behaviour for a role should go and
how-to access that behaviour.

The flip side of this is that models end up with redundant conditional
branches to do x for RoleA or y for RoleB (for every model thats
affected by a Role). I would like to collapse these conditional
branches and attempt a more polymorphic approach by extracting a class
representative of each role, and simply calling the method in
question. e.g:

  FiscalAssociate.new(user).invoices
  FiscalAdmin.new(user).invoices

Rather than the following approach. Keep in mind that roles hardly
ever are limited to one model. Consider having the case statement in
20 models. Where's the redundancy now?

   Invoice.by_role(<role_name>)

   def by_role(role
       case role
       when "associate"
        ....
       when "admin"
        ...
   end


--
Zach Dennis
http://www.continuousthinking.com
http://www.mutuallyhuman.com
85d99e7678d8720f6e00ab0f60fe6ea9?d=identicon&s=25 Andrew Premdas (Guest)
on 2009-03-03 21:03
(Received via mailing list)
Zach

By saying that models need the following case statement

  def by_role(role
   case role
     when "associate"
       ....
     when "admin"
       ...
  end

your implying that the model is represented in a different way depending
on
the role. So in your case of invoices the associate's view is different
from
the administrators. If this is the case then you have identified two
distinct resources!

Currently you are representing these two resources with one url e.g.
/invoices

However if you think of these as seperate resources then you have two
url's

/admin_invoices /associatie_invoices

Other url schems come fo mind /admin/invoices /invoices/admin etc.

Point is that you can implement these seperate resources in the standard
rails way and remove any need for case statements

class AdminInvoicesController
  before_filter must_be_admin

  def index
    Invoices.find # specify admin criteria here
  end
end

class AssociateInvoices
  before_filter must_be_associate

  def index
    Invoices.find # specify associate criteria here
  end
end

Andrew

2009/3/3 Zach Dennis <zach.dennis@gmail.com>
This topic is locked and can not be replied to.