From e648f1c57d164475ef9fff08f4e04f83c14d3cc7 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sun, 3 Jul 2022 05:40:11 +0200 Subject: [PATCH] Eliminate ThreadLocals We can nicely co-locate services with their users, eliminating one instance of centralization. Change-Id: Ibad16eb8241b5baa584782a2027ba0bd87b775ba Signed-off-by: Robert Varga --- .../MDSALDynamicAuthorizationFilter.java | 18 ++++++++++--- .../aaa/shiro/realm/MdsalRealm.java | 27 ++++++++++++++++--- .../shiro/web/env/AAAIniWebEnvironment.java | 10 +++---- .../aaa/shiro/web/env/ThreadLocals.java | 27 ------------------- .../MDSALDynamicAuthorizationFilterTest.java | 24 +++++------------ 5 files changed, 49 insertions(+), 57 deletions(-) delete mode 100644 aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java index 094ec29a0..4f6656e2e 100644 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java +++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java @@ -8,6 +8,7 @@ package org.opendaylight.aaa.shiro.realm; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Verify.verifyNotNull; import static java.util.Objects.requireNonNull; import com.google.common.collect.Iterables; @@ -25,7 +26,6 @@ import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import org.apache.shiro.subject.Subject; import org.apache.shiro.web.filter.authz.AuthorizationFilter; -import org.opendaylight.aaa.shiro.web.env.ThreadLocals; import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener; import org.opendaylight.mdsal.binding.api.DataBroker; import org.opendaylight.mdsal.binding.api.DataTreeIdentifier; @@ -36,6 +36,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev1 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.authorization.policies.Policies; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.permission.Permissions; import org.opendaylight.yangtools.concepts.ListenerRegistration; +import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -57,13 +58,24 @@ public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter private static final DataTreeIdentifier AUTHZ_CONTAINER = DataTreeIdentifier.create( LogicalDatastoreType.CONFIGURATION, InstanceIdentifier.create(HttpAuthorization.class)); + private static final ThreadLocal DATABROKER_TL = new ThreadLocal<>(); + private final DataBroker dataBroker; private ListenerRegistration reg; private volatile ListenableFuture> authContainer; public MDSALDynamicAuthorizationFilter() { - dataBroker = requireNonNull(ThreadLocals.DATABROKER_TL.get()); + this(verifyNotNull(DATABROKER_TL.get(), "MDSALDynamicAuthorizationFilter loading not prepared")); + } + + public MDSALDynamicAuthorizationFilter(final DataBroker dataBroker) { + this.dataBroker = requireNonNull(dataBroker); + } + + public static Registration prepareForLoad(final DataBroker dataBroker) { + DATABROKER_TL.set(requireNonNull(dataBroker)); + return DATABROKER_TL::remove; } @Override @@ -71,7 +83,7 @@ public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter try (ReadTransaction tx = dataBroker.newReadOnlyTransaction()) { authContainer = tx.read(AUTHZ_CONTAINER.getDatastoreType(), AUTHZ_CONTAINER.getRootIdentifier()); } - this.reg = dataBroker.registerDataTreeChangeListener(AUTHZ_CONTAINER, this); + reg = dataBroker.registerDataTreeChangeListener(AUTHZ_CONTAINER, this); return super.processPathConfig(path, config); } diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java index b5cfabba7..a160d8024 100644 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java +++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java @@ -7,6 +7,7 @@ */ package org.opendaylight.aaa.shiro.realm; +import static com.google.common.base.Verify.verifyNotNull; import static java.util.Objects.requireNonNull; import com.google.common.collect.Iterables; @@ -31,7 +32,6 @@ import org.opendaylight.aaa.api.shiro.principal.ODLPrincipal; import org.opendaylight.aaa.shiro.principal.ODLPrincipalImpl; import org.opendaylight.aaa.shiro.realm.util.TokenUtils; import org.opendaylight.aaa.shiro.realm.util.http.header.HeaderUtils; -import org.opendaylight.aaa.shiro.web.env.ThreadLocals; import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener; import org.opendaylight.mdsal.binding.api.DataBroker; import org.opendaylight.mdsal.binding.api.DataTreeIdentifier; @@ -44,6 +44,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev1 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.authentication.Roles; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.authentication.Users; import org.opendaylight.yangtools.concepts.ListenerRegistration; +import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -60,14 +61,24 @@ public class MdsalRealm extends AuthorizingRealm implements Destroyable { private static final DataTreeIdentifier AUTH_TREE_ID = DataTreeIdentifier.create( LogicalDatastoreType.CONFIGURATION, InstanceIdentifier.create(Authentication.class)); + private static final ThreadLocal PASSWORD_HASH_SERVICE_TL = new ThreadLocal<>(); + private static final ThreadLocal DATABROKER_TL = new ThreadLocal<>(); + private final PasswordHashService passwordHashService; private final ListenerRegistration reg; private volatile ListenableFuture> authentication; public MdsalRealm() { - this.passwordHashService = requireNonNull(ThreadLocals.PASSWORD_HASH_SERVICE_TL.get()); - final DataBroker dataBroker = requireNonNull(ThreadLocals.DATABROKER_TL.get()); + this(verifyLoad(PASSWORD_HASH_SERVICE_TL), verifyLoad(DATABROKER_TL)); + } + + private static T verifyLoad(final ThreadLocal threadLocal) { + return verifyNotNull(threadLocal.get(), "MdsalRealm not prepared for loading"); + } + + public MdsalRealm(final PasswordHashService passwordHashService, final DataBroker dataBroker) { + this.passwordHashService = requireNonNull(passwordHashService); try (ReadTransaction tx = dataBroker.newReadOnlyTransaction()) { authentication = tx.read(AUTH_TREE_ID.getDatastoreType(), AUTH_TREE_ID.getRootIdentifier()); @@ -79,6 +90,16 @@ public class MdsalRealm extends AuthorizingRealm implements Destroyable { LOG.info("MdsalRealm created"); } + public static Registration prepareForLoad(final PasswordHashService passwordHashService, + final DataBroker dataBroker) { + PASSWORD_HASH_SERVICE_TL.set(requireNonNull(passwordHashService)); + DATABROKER_TL.set(requireNonNull(dataBroker)); + return () -> { + PASSWORD_HASH_SERVICE_TL.remove(); + DATABROKER_TL.remove(); + }; + } + private void onAuthenticationChanged(final Collection> changes) { final Authentication newVal = Iterables.getLast(changes).getRootNode().getDataAfter(); LOG.debug("Updating authentication information to {}", newVal); diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java index f085e5694..a5337cba7 100644 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java +++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java @@ -19,6 +19,8 @@ import org.opendaylight.aaa.api.TokenStore; import org.opendaylight.aaa.api.password.service.PasswordHashService; import org.opendaylight.aaa.cert.api.ICertificateManager; import org.opendaylight.aaa.shiro.realm.KeystoneAuthRealm; +import org.opendaylight.aaa.shiro.realm.MDSALDynamicAuthorizationFilter; +import org.opendaylight.aaa.shiro.realm.MdsalRealm; import org.opendaylight.aaa.shiro.realm.MoonRealm; import org.opendaylight.aaa.shiro.realm.TokenAuthRealm; import org.opendaylight.aaa.tokenauthrealm.auth.TokenAuthenticators; @@ -94,13 +96,12 @@ class AAAIniWebEnvironment extends IniWebEnvironment { @Override public void init() { - ThreadLocals.DATABROKER_TL.set(dataBroker); - ThreadLocals.PASSWORD_HASH_SERVICE_TL.set(passwordHashService); try ( + var filterLoad = MDSALDynamicAuthorizationFilter.prepareForLoad(dataBroker); var keyStoneLoad = KeystoneAuthRealm.prepareForLoad(certificateManager); + var mdsalLoad = MdsalRealm.prepareForLoad(passwordHashService, dataBroker); var moonLoad = MoonRealm.prepareForLoad(servletSupport); var tokenAuthLoad = TokenAuthRealm.prepareForLoad(authenticationService, tokenAuthenticators, tokenStore)) { - // Initialize the Shiro environment from clustered-app-config final Ini ini = createIniFromClusteredAppConfig(shiroConfiguration); setIni(ini); @@ -108,9 +109,6 @@ class AAAIniWebEnvironment extends IniWebEnvironment { super.init(); return null; }); - } finally { - ThreadLocals.DATABROKER_TL.remove(); - ThreadLocals.PASSWORD_HASH_SERVICE_TL.remove(); } } } diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java deleted file mode 100644 index 469c7fbfc..000000000 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Copyright (c) 2018 Inocybe Technologies and others. All rights reserved. - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License v1.0 which accompanies this distribution, - * and is available at http://www.eclipse.org/legal/epl-v10.html - */ -package org.opendaylight.aaa.shiro.web.env; - -import org.opendaylight.aaa.api.password.service.PasswordHashService; -import org.opendaylight.mdsal.binding.api.DataBroker; - -/** - * Holds ThreadLocal variables used to indirectly inject instances into classes that are instantiated by the Shiro - * lib. Not ideal but a necessary evil to avoid static instances. - * - * @author Thomas Pantelis - */ -public final class ThreadLocals { - public static final ThreadLocal DATABROKER_TL = new ThreadLocal<>(); - - public static final ThreadLocal PASSWORD_HASH_SERVICE_TL = new ThreadLocal<>(); - - private ThreadLocals() { - // Hidden on purpose - } -} diff --git a/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java b/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java index 4edb06ba7..c662fa08c 100644 --- a/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java +++ b/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java @@ -24,9 +24,7 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import org.apache.shiro.subject.Subject; -import org.junit.Before; import org.junit.Test; -import org.opendaylight.aaa.shiro.web.env.ThreadLocals; import org.opendaylight.mdsal.binding.api.DataBroker; import org.opendaylight.mdsal.binding.api.ReadTransaction; import org.opendaylight.mdsal.common.api.ReadFailedException; @@ -42,10 +40,6 @@ import org.opendaylight.yangtools.yang.common.Uint32; @SuppressWarnings("checkstyle:AbbreviationAsWordInName") public class MDSALDynamicAuthorizationFilterTest { - @Before - public void setup() { - } - private static DataBroker mockDataBroker(final Object readData) { final ReadTransaction readOnlyTransaction = mock(ReadTransaction.class); @@ -65,18 +59,12 @@ public class MDSALDynamicAuthorizationFilterTest { private static MDSALDynamicAuthorizationFilter newFilter(final Subject subject, final DataBroker dataBroker) throws ServletException { - ThreadLocals.DATABROKER_TL.set(dataBroker); - MDSALDynamicAuthorizationFilter ret; - try { - ret = new MDSALDynamicAuthorizationFilter() { - @Override - protected Subject getSubject(final ServletRequest request, final ServletResponse servletResponse) { - return subject; - } - }; - } finally { - ThreadLocals.DATABROKER_TL.remove(); - } + final var ret = new MDSALDynamicAuthorizationFilter(dataBroker) { + @Override + protected Subject getSubject(final ServletRequest request, final ServletResponse servletResponse) { + return subject; + } + }; ret.processPathConfig("test-path","test-config"); return ret; -- 2.36.6