access control code questions

Dmitri Dolguikh dmitri at redhat.com
Fri May 14 14:18:09 UTC 2010


On 10-05-13 7:33 PM, Justin Harris wrote:
> ----- 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.
>    
+1
>    
>> * 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.
>    
+1
>    
>> * 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
>    
sure.
>    
>> * 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.
>>      
I'm not very keen on this design myself; I wanted to avoid unnecessary 
introspection and/or a bunch of instanceof checks, hence the 
visitor-like approach. I do not like that I'm asking the object itself 
about its access rights.
>>    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
>>      
I think we can execute a native query in place of relying on hibernate 
to traverse the relationships.
> 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.
>    
I like less casting :).
>
>    
>> -- 
>> 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
>>      
> _______________________________________________
> candlepin mailing list
> candlepin at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/candlepin
>    




More information about the candlepin mailing list