[
https://issues.apache.org/jira/browse/CASSANDRA-14662?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16590118#comment-16590118
]
Sam Tunnicliffe commented on CASSANDRA-14662:
---------------------------------------------
bq. Makes it possible to specify a CacheLoader rather than just a Function,
allowing you to have a get/load that throws exceptions.
I don't believe this is the right thing to do as {{cache::get}} will wrap the
checked exception thrown from the loader in an unchecked exception which any
user of the cache will have to handle. If they don't, these will leak into the
client response, potentially revealing implementation details of the
authentication backend. A simpler solution would be to just have the load
function catch any checked exceptions internally and rethrow an appropriate
subclass of {{CassandraException}}, which are unchecked.
Also, as you noted, this change is also problematic in cases where the cache
has been disabled, forcing the caller to supply both a {{CacheLoader}} and a
function in case the caching functionality is ever disabled. Effectively, any
implementation would need to do this 100% of the time as caches can be disable
via JMX at runtime.
An alternative would be to change {{AuthCache}} to take *only* a
{{CacheLoader}}. As this is a functional interface, most cases could continue
to supply just the function unchanged, but {{cache::get}} would need to be
modified to call {{cacheLoader::load}} and either propagate or handle checked
exceptions as above. This is also suboptimal, as {{AuthCache}} itself doesn't
have enough context of the operation to handle these appropriately (i.e. is it
an authn error, or authz etc).
bq. Use AuthCache on its own rather than extending it for each use case
(invalidate(K) moved to be part of MBean)
Parameterizing the MBean interface is a good idea and moving the
{{invalidate(K)}} methods there is an improvement. Removing the deprecated
MBean interfaces is also cool.
I am -1 on removing the specializations of {{AuthCache}} though, for the
reasons related to the next point.
bq. Provided a builder that uses sane defaults so we don't have unnecessary
repeated code everywhere
I don't agree that the builder pattern introduced here removes any duplication
or makes the calling code any clear. For instance, compare the current trunk
with the proposed:
{code}
private static final PermissionsCache permissionsCache = new
PermissionsCache(DatabaseDescriptor.getAuthorizer());
private static final NetworkAuthCache networkAuthCache = new
NetworkAuthCache(DatabaseDescriptor.getNetworkAuthorizer());
{code}
{code}
private static final AuthCache<Pair<AuthenticatedUser, IResource>,
Set<Permission>> permissionsCache = new
AuthCache.Builder<Pair<AuthenticatedUser, IResource>, Set<Permission>>()
.withCacheLoader((p) ->
DatabaseDescriptor.getAuthorizer().authorize(p.left, p.right))
.withCacheEnabled(() ->
DatabaseDescriptor.getAuthorizer().requireAuthorization())
.build("PermissionsCache");
private static final AuthCache<RoleResource, DCPermissions> networkAuthCache =
new AuthCache.Builder<RoleResource, DCPermissions>()
.withCacheLoader(DatabaseDescriptor.getNetworkAuthorizer()::authorize)
.withCacheEnabled(() ->
DatabaseDescriptor.getAuthenticator().requireAuthentication())
.build("NetworkAuthCache");
{code}
With the specialized impls the boilerplate is hidden behind the constructor, so
the caller doesn't need to know about it.
The builder pattern is most useful when constructors have a mix of mandatory
and optional arguments. {{AuthCache}} itself doesn't have any optional
arguments, but some of the values are domain specific and so the
implementations can supply them.
Making things like the validity & refresh values optional and defaulting them
to an arbitrary set of config props is not an improvement.
So, I'm good with the changes to the MBeans but I'm not super keen on any of
the other bits. Can you verify that the problems you had with the custom
{{IAuthenticator}} can't be solved by modifying the load function as described?
> Refactor AuthCache
> ------------------
>
> Key: CASSANDRA-14662
> URL: https://issues.apache.org/jira/browse/CASSANDRA-14662
> Project: Cassandra
> Issue Type: Improvement
> Components: Auth
> Reporter: Kurt Greaves
> Assignee: Kurt Greaves
> Priority: Major
> Labels: security
> Fix For: 4.x
>
>
> When building an LDAP IAuthenticator plugin I ran into a few issues when
> trying to reuse the AuthCache similar to how PasswordAuthenticator implements
> it. Most of the problems stemmed from the underlying cache being inaccessible
> and not being able to override {{initCache}} properly.
> Anyway, I've had a stab at refactoring AuthCache with the following
> improvements:
> # Make it possible to extend and override all necessary methods (initCache,
> init, validate)
> # Makes it possible to specify a {{CacheLoader}} rather than just a
> {{Function}}, allowing you to have a get/load that throws exceptions.
> # Use AuthCache on its own rather than extending it for each use case
> ({{invalidate(K)}} moved to be part of MBean)
> # Provided a builder that uses sane defaults so we don't have unnecessary
> repeated code everywhere
> The refactor made all the extensions of AuthCache unnecessary, so I've
> simplified those cases to use AuthCache and removed any classes extending
> AuthCache. I also removed some noop compatibility classes that were marked to
> be removed in 4.0.
> Also added some tests in AuthCacheTest.
> |[trunk|https://github.com/apache/cassandra/compare/trunk...kgreav:authcache]|
> |[utests|https://circleci.com/gh/kgreav/cassandra/206]|
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
