Security is one of those exceptions to the rules about ThreadLocal. Almost
every implementation uses ThreadLocal to stash the current executing
context. Generally then there is a static class that gets the current
context. To test you should be able to just push your own mocked context
into the context. Not familiar with Shiro but I have done this with a few
other JAAS implementations.
-Jake
On Mon, Jul 25, 2016 at 5:33 PM Jinmei Liao <jil...@pivotal.io> wrote:
> Shiro relies heavily on ThreadLocal. GeodeSecurityUtils is a wrapper around
> Shiro. That's where all the static usage stems from. There is a trade-off
> if we pass some sort session object around: calling code would need to keep
> a reference of it and knows when/where/which to pass it down.
>
> On Mon, Jul 25, 2016 at 5:21 PM, Dan Smith <dsm...@pivotal.io> wrote:
>
> > IMHO static helper methods like those in Bytes are fine. We would never
> > want to substitute a mock implementation of one of those methods and they
> > don't have any collaborators.
> >
> > Just glancing at GeodeSecurityUtils, what I don't like is that it's
> relying
> > heavily on mutable static singletons (securityManager, postProcessor) and
> > thread locals. I think we should avoid using singletons and thread locals
> > in geode code, because it's too easy to leave junk lying around and it
> also
> > makes code hard to test. In this case maybe it would make more sense to
> > pass some sort of session object around?
> >
> > -Dan
> >
> > On Mon, Jul 25, 2016 at 4:26 PM, Kirk Lund <kl...@pivotal.io> wrote:
> >
> > > I was trying to describe any Java class in Geode which provides feature
> > > functionality that should be unit tested and integration tested, but is
> > > either currently implemented as a static class or which collaborates
> > with a
> > > non-trivial component currently implemented as a static class.
> > >
> > > I wasn't describing the simple static classes like Byte, StringUtils,
> > > IOUtils. These are trivial and not generally considered to be an
> > important
> > > collaborator requiring a mock.
> > >
> > > Using Mockito allows us to test the interaction between a class under
> > test
> > > and its collaborators. To facilitate this, we need the collaborator to
> be
> > > defined as an interface or abstract class.
> > >
> > > This specifically came about on my team because we have been unit
> testing
> > > GMSAuthenticator and lots of Client Commands that interact with
> > Integrated
> > > Security. We want to include unit tests to verify that the class under
> > test
> > > correctly interacts with its collaborators including Integrated
> Security
> > > (GEODE-17). For this reason, we want to transform GeodeSecurityUtil
> into
> > a
> > > non-static class that implements an interface that can be mocked.
> > >
> > > Hopefully everyone agrees that we need to make a concerted effort to
> > write
> > > lots of new fast-running unit tests that use mocks. This includes old
> and
> > > new code.
> > >
> > > In general, actual features implemented in static classes is common in
> > > Geode but it inhibits unit testing and thus should change as we
> refactor
> > > the code towards testability.
> > >
> > > This thread started as me describing a simple pattern to facilitate
> unit
> > > testing against static classes that for whatever reason, we want to
> hold
> > > off refactoring them but we don't want to hold off writing unit tests
> for
> > > them as an important collaborator class.
> > >
> > > Does this make sense? Is everyone on the same page regarding the value
> of
> > > unit testing and refactoring to testability & maintainability?
> > >
> > > -Kirk
> > >
> > > On Wednesday, July 20, 2016, Anthony Baker <aba...@pivotal.io> wrote:
> > >
> > > > Great discussion! I’ve got two questions:
> > > >
> > > > 1) What do you think of Bytes.java
> > > > (:geode-core:com.gemstone.gemfire.internal.util)? Is it a reasonable
> > use
> > > > of the Helper / Utility class pattern? If not, how would you change
> > it?
> > > > Creating a new instance for each use seems inefficient and using a
> > > > Singleton is just a different way to do statics.
> > > >
> > > > 2) Is GeodeSecurityUtil of the same category as Bytes.java or is it
> > > > something more?
> > > >
> > > > Anthony
> > > >
> > > >
> > > > > On Jul 20, 2016, at 10:41 AM, Jinmei Liao <jil...@pivotal.io
> > > > <javascript:;>> wrote:
> > > > >
> > > > > Wouldn't the change be too intrusive? Now any class that uses
> > security
> > > > > would need to have this additional constructor, and if a no-arg
> > > > constructor
> > > > > is available, it's error prone that the class will be used without
> > > > > security, and if we delete the no-arg constructor, that would
> cause a
> > > lot
> > > > > of chain reaction.
> > > > >
> > > > > The usage of shiro itself is static (all the state information is
> in
> > > the
> > > > > thread), which, in my opinion is one of the selling points of Shiro
> > > since
> > > > > it greatly simplifies the usage of it. That's why GeodeSecurityUtil
> > > > follows
> > > > > this path. If no security is configured, calls into any of the
> method
> > > in
> > > > > GeodeSecurityUtil would be a no-op. And each VM would need only one
> > > > > instance of GeodeSecurityUtil so static makes sense here IMHO.
> > > > >
> > > > > For testing purposes, if you are not testing security in your test,
> > the
> > > > > calls into GeodeSecurityUtil/Shiro would act like non-existent. If
> > you
> > > > need
> > > > > special behavior of GeodeSecurityUtil, just init it with a property
> > > > object
> > > > > that would give you the behavior you want in your test like this,
> but
> > > > then
> > > > > we've had tests that already covers the security aspect of all the
> > > > calling
> > > > > code, so your test would not need to.
> > > > >
> > > > > @Test
> > > > > public void testInitialSecurityFlags() {
> > > > > // initial state of GeodeSecurityUtil
> > > > > assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
> > > > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > > > assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
> > > > > }
> > > > >
> > > > > @Test
> > > > > public void testInitWithSecurityManager() {
> > > > > properties.setProperty(SECURITY_MANAGER,
> > > > > "org.apache.geode.security.templates.SampleSecurityManager");
> > > > > GeodeSecurityUtil.initSecurity(properties);
> > > > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > > > assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
> > > > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > > > }
> > > > >
> > > > > @Test
> > > > > public void testInitWithClientAuthenticator()
> > > > > {
> > > > > properties.setProperty(SECURITY_CLIENT_AUTHENTICATOR,
> > "org.abc.test");
> > > > > GeodeSecurityUtil.initSecurity(properties);
> > > > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > > > assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
> > > > > }
> > > > >
> > > > > @Test
> > > > > public void testInitWithPeerAuthenticator()
> > > > > {
> > > > > properties.setProperty(SECURITY_PEER_AUTHENTICATOR,
> "org.abc.test");
> > > > > GeodeSecurityUtil.initSecurity(properties);
> > > > > assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
> > > > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > > > }
> > > > >
> > > > > @Test
> > > > > public void testInitWithShiroAuthenticator()
> > > > > {
> > > > > properties.setProperty(SECURITY_SHIRO_INIT, "shiro.ini");
> > > > > GeodeSecurityUtil.initSecurity(properties);
> > > > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > > > assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
> > > > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > > > }
> > > > >
> > > > >
> > > > > On Wed, Jul 20, 2016 at 10:16 AM, Kirk Lund <kl...@pivotal.io
> > > > <javascript:;>> wrote:
> > > > >
> > > > >> To avoid adding boilerplate code, we could just avoid writing any
> > new
> > > > >> static methods or classes of static methods. Remember static is
> bad
> > in
> > > > OO
> > > > >> and is one of the main things that prevents true, good unit
> testing
> > > > because
> > > > >> you can't replace the class or the method with a fake of any sort
> > > (mock,
> > > > >> spy, or dummy).
> > > > >>
> > > > >> I would vote to eliminate all statics and adopt a policy of no new
> > > > statics
> > > > >> and follow:
> > > > >>
> > > > >> C.1) change all methods in GeodeUtils to be non-static and pass in
> > an
> > > > >> instance of GeodeUtils to the class constructor of any class that
> > will
> > > > use
> > > > >> it
> > > > >>
> > > > >> -Kirk
> > > > >>
> > > > >>
> > > > >> On Wed, Jul 20, 2016 at 9:57 AM, Jinmei Liao <jil...@pivotal.io
> > > > <javascript:;>> wrote:
> > > > >>
> > > > >>> One other to bear in mind is to avoid adding too much boiler code
> > > into
> > > > >> the
> > > > >>> production solely for the purpose of testing. I am typically a OO
> > > > person,
> > > > >>> but I wouldn't sacrifice code clarity and readability for it.
> > > > >>>
> > > > >>> On Fri, Jul 15, 2016 at 12:14 PM, Kirk Lund <kl...@pivotal.io
> > > > <javascript:;>> wrote:
> > > > >>>
> > > > >>>> We recently introduced GeodeSecurityUtil for GEODE-17, and I'm
> > > looking
> > > > >>> into
> > > > >>>> ways to refactor this class or the classes using it to better
> > > > >> facilitate
> > > > >>>> unit testing of those classes. I'm hoping that others on this
> list
> > > > will
> > > > >>>> chime in with ideas and opinions. GeodeSecurityUtil itself is a
> > > > >>> collection
> > > > >>>> of public static methods that use Apache Shiro and callback APIs
> > > (such
> > > > >> as
> > > > >>>> SecurityManager) in Geode.
> > > > >>>>
> > > > >>>> First, a few general pre-assertions:
> > > > >>>> A.1) we want to embrace clean coding and solid OOP coding
> > practices,
> > > > >>>> A.2) we want to write fast running unit tests that fully isolate
> > the
> > > > >>> class
> > > > >>>> under test (this typically uses mocking),
> > > > >>>> A.3) we want to be able to unit test any code in a class even if
> > > it's
> > > > >> in
> > > > >>> a
> > > > >>>> method that invokes public static methods in another class,
> > > > >>>> A.4) simple mocking with Mockito is much preferred over using
> > > > >> Powermock,
> > > > >>>> A.5) we want to also write integration tests but the emphasis is
> > on
> > > > >> fast
> > > > >>>> unit tests.
> > > > >>>>
> > > > >>>> There are quite a lot of procedurally static classes already in
> > the
> > > > >> Geode
> > > > >>>> codebase, so I'm hoping some of these ideas will be applicable
> to
> > > > >> helping
> > > > >>>> out others working on other areas of the code as well.
> > > > >>>>
> > > > >>>> Any class that has calls to a collaborator with public static
> > > methods
> > > > >>> makes
> > > > >>>> the unit testing of that class much more difficult:
> > > > >>>>
> > > > >>>> B.1) you cannot spy the static util class to verify that the
> class
> > > > >> under
> > > > >>>> test invoked it with proper parameters
> > > > >>>>
> > > > >>>> B.2) you cannot mock the static util class to return expected
> > values
> > > > to
> > > > >>> the
> > > > >>>> class under test
> > > > >>>>
> > > > >>>> If the following is a util type class:
> > > > >>>>
> > > > >>>> public class GeodeUtils {
> > > > >>>>
> > > > >>>> public static void persist(String key) {
> > > > >>>> ...
> > > > >>>> }
> > > > >>>>
> > > > >>>> public static String getValue() {
> > > > >>>> ...
> > > > >>>> }
> > > > >>>> }
> > > > >>>>
> > > > >>>> ...then there are two potential ways to facilitate unit testing
> of
> > > > >>> classes
> > > > >>>> that depend on GeodeUtils:
> > > > >>>>
> > > > >>>> C.1) change all methods in GeodeUtils to be non-static and pass
> in
> > > an
> > > > >>>> instance of GeodeUtils to the class constructor of any class
> that
> > > will
> > > > >>> use
> > > > >>>> it
> > > > >>>>
> > > > >>>> C.2) wrap these statics with an interface and default
> > implementation
> > > > >> that
> > > > >>>> delegates to the static calls:
> > > > >>>>
> > > > >>>> public interface GeodeService {
> > > > >>>>
> > > > >>>> void persist(String key);
> > > > >>>>
> > > > >>>> String getValue();
> > > > >>>> }
> > > > >>>>
> > > > >>>> public class DefaultGeodeService implements GeodeService {
> > > > >>>>
> > > > >>>> public void persist(String key) {
> > > > >>>> GeodeUtils.persist(key);
> > > > >>>> }
> > > > >>>>
> > > > >>>> public String getValue() {
> > > > >>>> return GeodeUtils.getValue();
> > > > >>>> }
> > > > >>>> }
> > > > >>>>
> > > > >>>> public class DurableOperation {
> > > > >>>>
> > > > >>>> private final Properties config;
> > > > >>>> private final GeodeService geodeService;
> > > > >>>>
> > > > >>>> public DurableOperation(Properties config) {
> > > > >>>> this(config , new DefaultGeodeService());
> > > > >>>> }
> > > > >>>>
> > > > >>>> DurableOperation(Properties config, GeodeService geodeService)
> {
> > > > >>>> this.config = config;
> > > > >>>> this.geodeService = geodeService;
> > > > >>>> }
> > > > >>>>
> > > > >>>> public void perform(String key) throws IOException {
> > > > >>>> ...
> > > > >>>> this.geodeService.persist(key);
> > > > >>>> ...
> > > > >>>> }
> > > > >>>> }
> > > > >>>>
> > > > >>>> public class DurableOperationTest {
> > > > >>>>
> > > > >>>> @Test
> > > > >>>> public void shouldPersistKey() throws Exception {
> > > > >>>> // given
> > > > >>>> Properties config = generateFancyOperationConfig();
> > > > >>>> GeodeService spyGeodeService = spy(GeodeService.class);
> > > > >>>> when(spyGeodeService.getValue()).thenReturn("bar");
> > > > >>>> DurableOperation durableOperation = new
> > DurableOperation(config,
> > > > >>>> spyGeodeService);
> > > > >>>>
> > > > >>>> // when
> > > > >>>> durableOperation.perform("foo");
> > > > >>>>
> > > > >>>> // then
> > > > >>>> verify(spyGeodeService, times(1)).persist("foo");
> > > > >>>> }
> > > > >>>> }
> > > > >>>>
> > > > >>>> C.3) do C.2 until C.1 can occur (what prevents us from just
> doing
> > > > C.1?)
> > > > >>>>
> > > > >>>> I believe that in general we want to use the solution in C.1
> where
> > > > >>>> possible.
> > > > >>>>
> > > > >>>> To foster some discussion:
> > > > >>>>
> > > > >>>> D.1) do we agree on the pre-assertions I listed in A.1-5?
> > > > >>>> D.2) do we have procedurally static classes that cannot or
> should
> > > not
> > > > >> be
> > > > >>>> changed from static to non-static as described by C.1?
> > > > >>>> D.3) are there any other approaches that will better facilitate
> > unit
> > > > >>>> testing of classes that depend on static collaborators?
> > > > >>>>
> > > > >>>> I'd like to ultimately encourage the Geode dev community to
> avoid
> > > > >>> creating
> > > > >>>> new classes that consist of static methods and state. I believe
> we
> > > can
> > > > >> do
> > > > >>>> this and that it's for the best.
> > > > >>>>
> > > > >>>> -Kirk
> > > > >>>>
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>> --
> > > > >>> Cheers
> > > > >>>
> > > > >>> Jinmei
> > > > >>>
> > > > >>
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Cheers
> > > > >
> > > > > Jinmei
> > > >
> > > >
> > >
> >
>
>
>
> --
> Cheers
>
> Jinmei
>
