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