[
https://issues.apache.org/jira/browse/CASSANDRA-14662?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16591490#comment-16591490
]
Kurt Greaves commented on CASSANDRA-14662:
------------------------------------------
Thanks for the quick review.
bq. 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?
Yep thanks, that's a worthwhile solution. Will remove the unnecessary
CacheLoader options.
bq. With the specialized impls the boilerplate is hidden behind the
constructor, so the caller doesn't need to know about it.
I don't really agree here. There's a specialised implementation for each usage
of the cache, so I don't think we really make anything any clearer. All we've
done is add a layer of indirection. If the implementations were used more than
once I think that could make sense, but none of them are, and we don't even use
their {{get()}} calls more than once.
{quote}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.
{quote}
Why is having defaults not an improvement? They are literally the same for
every usage/implementation of the AuthCache in the codebase which means they
aren't arbitrary for all the existing use cases. Seems like a case for sane
defaults to me?
I mean, I'm not super tied to the builder, but I really don't see the point in
having the exact same code in four different places so that we can claim that
the caller benefits by not having to "know" about it, especially because we
haven't found reason to expose these implementations anyway so it's always just
an implementation detail of the (single) class that uses each one.
> 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: [email protected]
For additional commands, e-mail: [email protected]
