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.
* 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?
* 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]
* 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
--
jesus m. rodriguez | jesusr(a)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 |
+---------------------------------------------+