access control code questions

Justin Harris jharris at redhat.com
Thu May 13 22:33:24 UTC 2010


----- jesusr at redhat.com wrote:

> All,
> 
> 
> While investigating the ConsumerTest failure (thx Dmitri for fixing
> it)
> I started looking at the access control code and have some questions.
> 
> AccessControlInterceptor.java
> ------------------------------
> * in invoke(MethodInvocation invocation) we cast invocation.getThis()
> to
>   a AbstractHibernateCurator so we can get the entityType.
> 
>   Can the invocation type be anything other than a Curator? If so, we
>   need to protect against this.

If we want to generalize this later, yes.  But currently we are only binding this
interceptor to AbstractHibernateCurator subclasses, so this is fine for now.

> 
> * the private method crudAccessControl(Object) takes in an object but
>   always assumes that the passed in object is of type
> AccessControlEnforced.
>   Any reason this method doesn't just take in AccessControlEnforced
>   instead of Object?

Agreed - this can probably be cast earlier in the process.

> 
> * inside the same method we make a call to
> ConsumerPrincipal.consumer(),
>   shouldn't that be getConsumer()? I'm not a big fan of
> getters/setters
>   but are we going down the route of not using them now?
>   http://zeusville.wordpress.com/2007/07/25/java-getterssetters/ [1]

 +1

> 
> * this led me to the model classes which implement
> AccessControlEnforced,
>   they all seem to call a static class: AccessControlValidator (ACV)
> which
>   has a bunch of shouldGrantAccess(SomeModelObject accessed,
> (Consumer|Owner)).
> 
>   It seems like we're mixing two styles here, 1) where the model
> objects
>   are smart i.e. know how to answer if they have access to something
>   2) using a helper class to make that decision.
> 
>   I'd prefer we pick one. Either have the model object inspect itself
>   to answer the question shouldGrantAccessTo(Owner|Consumer) OR
>   have the Interceptor call the ACV directly passing
>   in the AccessControlEnforced object.
> 
>   The other concern with the current approach is the ACV cracks open 
>   the model object to determine the answer:
> 
>     public static boolean shouldGrantAccess(Pool accessed, Consumer
> consumer) {
>       return accessed.getOwner().getConsumers().contains(consumer);
>     }
> 
> * Lastly the above code scares me too because of Hibernate.
> 
>      accessed.getOwner().getConsumers().contains(consumer);
> 
>   Doesn't this get the Pool's Owner, then gets loads ALL of the
> Owner's
>   Consumers (fully populated object), to look in the Java Set for a
> given
>   Consumer.  When in reality you really want a simple select statement
> like
> 
>     select c.id from cp_pool p, cp_consumer c
>     where p.owner_id = X AND
>       and c.owner_id = p.owner_id
>       and c.id = Y
> 
>   I have *NOT* verified what queries Hibernate will generate, so I
> might 
>   be on crack. I do know that this bit me before where it looked like
>   a simple java call, and it was a nasty SQL mess underneath.
> 
> [1] shameless self-promotion

It seems to me that it is the principal that defines who
is making the request, and so permission checks should be on the principal to see if they
can access a certain entity.  This allows for other types of identifying things to define their permissions,
and also can cut out some of the casting assumptions made in the interceptors.


> 
> -- 
> jesus m. rodriguez          | jesusr at redhat.com
> principal software engineer | irc: zeus
> red hat systems management  | 919.754.4413 (w)
> rhce # 805008586930012      | 919.623.0080 (c)
> +---------------------------------------------+
> |   "Those who cannot remember the past       |
> |    are condemned to repeat it."             |
> |                        -- George Santayana  |
> +---------------------------------------------+
> 
> _______________________________________________
> candlepin mailing list
> candlepin at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/candlepin



More information about the candlepin mailing list