pjfanning commented on code in PR #8703:
URL: https://github.com/apache/hadoop/pull/8703#discussion_r4400908502
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SignerSecretProvider.java:
##########
@@ -24,20 +24,77 @@
* do more complicated things in the backend.
* See the RolloverSignerSecretProvider class for an implementation that
* supports rolling over the secret at a regular interval.
+ * <p>
+ * Implementations should override
+ * {@link #initialize(Properties, SecretProviderContext, long)}, which names no
+ * servlet type. The older {@link #init(Properties, ServletContext, long)} is
+ * still the entry point callers use and is still honoured when an
+ * implementation overrides it, so providers written against it keep working
+ * unchanged; it is deprecated and will be removed with the move to the jakarta
+ * servlet namespace.
+ * <p>
+ * One consequence for code that reflects over these classes: the providers
+ * shipped here override
+ * {@link #initialize(Properties, SecretProviderContext, long)} rather than
+ * {@link #init(Properties, ServletContext, long)}, so init is no longer among
+ * their declared methods. Calls and overrides are unaffected - init is
+ * inherited from this class and resolves as it always did - but
+ * Class#getDeclaredMethod("init", ...) on a subclass such as
+ * {@link FileSignerSecretProvider} now raises NoSuchMethodException where it
+ * used to succeed. Class#getMethod, which searches superclasses, still finds
+ * it.
*/
@InterfaceStability.Unstable
@InterfaceAudience.Private
public abstract class SignerSecretProvider {
/**
- * Initialize the SignerSecretProvider
+ * Initialize the SignerSecretProvider.
+ * <p>
+ * The default implementation adapts the ServletContext to a
+ * {@link SecretProviderContext} and calls
+ * {@link #initialize(Properties, SecretProviderContext, long)}, so a
provider
+ * that overrides only that method is initialized correctly through this
+ * entry point. A provider that overrides this method instead is called
+ * directly, as before.
+ *
* @param config configuration properties
* @param servletContext servlet context
* @param tokenValidity The amount of time a token is valid for
* @throws Exception thrown if an error occurred
+ * @deprecated override
+ * {@link #initialize(Properties, SecretProviderContext, long)},
+ * which does not name a servlet type and so is unaffected by
+ * which servlet namespace the container provides.
*/
- public abstract void init(Properties config, ServletContext servletContext,
- long tokenValidity) throws Exception;
+ @Deprecated
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) throws Exception {
+ initialize(config, ServletSecretProviderContext.of(servletContext),
+ tokenValidity);
+ }
+
+ /**
+ * Initialize the SignerSecretProvider against an attribute store.
+ * <p>
+ * The default implementation throws, because a provider has to implement one
+ * of the two initialization methods. It is never reached by a provider that
+ * overrides the deprecated
+ * {@link #init(Properties, ServletContext, long)}, since callers go through
+ * that method and its override does not delegate here.
+ *
+ * @param config configuration properties
+ * @param context the attribute store to initialize against
+ * @param tokenValidity The amount of time a token is valid for
+ * @throws Exception thrown if an error occurred
+ */
+ public void initialize(Properties config, SecretProviderContext context,
Review Comment:
With `init` no longer abstract and `initialize` defaulting to a throw, the
compiler no longer forces a subclass to implement anything. A new provider that
implements neither method, or one that gets the `initialize` signature slightly
wrong without `@Override`, now compiles and fails with
`UnsupportedOperationException` at web-server startup instead of at build time.
I understand why (both entry points need a default), but it's worth noting in
the javadoc/release note, and a test covering the "implements neither" message
would pin the behaviour down.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/CertificateUtil.java:
##########
@@ -38,8 +38,37 @@ public class CertificateUtil {
* - the pem encoding from config without the header and footer
* @return RSAPublicKey the RSA public key
* @throws ServletException thrown if a processing error occurred
+ * @deprecated use {@link #toRSAPublicKey(String)}, which reports the
+ * {@link CertificateException} it is really raising instead of
+ * wrapping it in a servlet type. This method is kept so that
+ * existing callers still compile and behave as before, and will
+ * be removed with the move to the jakarta servlet namespace.
*/
- public static RSAPublicKey parseRSAPublicKey(String pem) throws
ServletException {
+ @Deprecated
+ public static RSAPublicKey parseRSAPublicKey(String pem)
+ throws ServletException {
+ try {
+ return toRSAPublicKey(pem);
+ } catch (CertificateException ce) {
+ // Report the exception toRSAPublicKey wrapped, not the wrapper, so that
+ // the cause a caller sees is the one this method has always reported:
+ // the CertificateException the parse itself raised. Wrapping the wrapper
+ // would add a level to the chain that was not there before.
+ Throwable cause = ce.getCause() == null ? ce : ce.getCause();
Review Comment:
`toRSAPublicKey` always throws a `CertificateException` with a non-null
cause, so the `ce.getCause() == null ? ce : ...` fallback can't run. The same
unwrap logic is also copied into `JWTRedirectAuthenticationHandler`. The
underlying issue is that `toRSAPublicKey` wraps a `CertificateException` in
another `CertificateException` only to change the message. A small
package-private helper (e.g. `static ServletException
toServletException(CertificateException)`) used by both call sites would keep
the cause chain in one place.
##########
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestSignerSecretProviderCompatibility.java:
##########
@@ -0,0 +1,228 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+
+import javax.servlet.ServletContext;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * A SignerSecretProvider written against the deprecated
+ * {@link SignerSecretProvider#init(Properties, ServletContext, long)} must
keep
+ * working without being recompiled, because implementations of it live outside
+ * this tree - signer.secret.provider takes a classname.
+ * <p>
+ * These tests pin that contract. They are the reason
+ * {@link SignerSecretProvider#init(Properties, ServletContext, long)} is a
+ * concrete bridge rather than a changed abstract method, and they go when it
+ * does.
+ */
+@SuppressWarnings("deprecation")
+public class TestSignerSecretProviderCompatibility {
+
+ private static final byte[] SECRET =
+ "secret".getBytes(StandardCharsets.UTF_8);
+
+ /**
+ * A provider as it would have been written before SecretProviderContext
+ * existed: it overrides init and takes a ServletContext.
+ */
+ private static final class LegacyProvider extends SignerSecretProvider {
+ private ServletContext seen;
+ private boolean initCalled;
+
+ @Override
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) {
+ this.seen = servletContext;
+ this.initCalled = true;
+ }
+
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ /**
+ * The same, extending RolloverSignerSecretProvider and chaining to super as
+ * such a provider is expected to.
+ */
+ private static final class LegacyRolloverProvider
+ extends RolloverSignerSecretProvider {
+ private boolean initCalled;
+
+ @Override
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) throws Exception {
+ this.initCalled = true;
+ super.init(config, servletContext, tokenValidity);
+ }
+
+ @Override
+ protected byte[] generateNewSecret() {
+ return SECRET;
+ }
+ }
+
+ /** A provider written against the servlet-free method. */
+ private static final class ContextProvider extends SignerSecretProvider {
+ private SecretProviderContext seen;
+
+ @Override
+ public void initialize(Properties config, SecretProviderContext context,
+ long tokenValidity) {
+ this.seen = context;
+ }
+
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ /** A provider that implements neither method. */
+ private static final class UninitializableProvider extends
SignerSecretProvider {
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ @Test
+ public void testLegacyProviderStillReceivesTheServletContext()
+ throws Exception {
+ ServletContext servletContext = mock(ServletContext.class);
+ LegacyProvider provider = new LegacyProvider();
+
+ provider.init(new Properties(), servletContext, 1000);
+
+ assertTrue(provider.initCalled, "the legacy override should have run");
+ assertSame(servletContext, provider.seen);
+ }
+
+ @Test
+ public void testLegacyRolloverProviderIsStillRolledOver() throws Exception {
+ LegacyRolloverProvider provider = new LegacyRolloverProvider();
+ try {
+ provider.init(new Properties(), mock(ServletContext.class), 100000);
+
+ // The override ran, and chaining to super still reached
+ // RolloverSignerSecretProvider.initialize, which seeds the secret. Were
+ // the bridge dispatched statically, this would silently be null.
+ assertTrue(provider.initCalled, "the legacy override should have run");
+ assertArrayEquals(SECRET, provider.getCurrentSecret());
+ } finally {
+ provider.destroy();
+ }
+ }
+
+ @Test
+ public void testContextProviderIsReachedThroughTheDeprecatedEntryPoint()
+ throws Exception {
+ Map<String, Object> attributes = new HashMap<>();
+ ServletContext servletContext = mock(ServletContext.class);
+ when(servletContext.getAttribute("a"))
Review Comment:
Nit: this `getAttribute` stub reads from `attributes`, but nothing ever
writes to that map, and the test only verifies `setAttribute`. Either drop the
stub, or have a `setAttribute` answer populate the map and assert that
`provider.seen.getAttribute("a")` reads the value back through the servlet
context.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SecretProviderContext.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+
+/**
+ * The attribute store a {@link SignerSecretProvider} is initialized against.
+ * <p>
+ * A SignerSecretProvider that has to share an object with the rest of the
+ * application it is embedded in - as {@link ZKSignerSecretProvider} shares its
+ * CuratorFramework client - reads and writes that object here. When the
+ * provider is running inside a servlet container the attributes are those of
+ * the ServletContext, so the sharing is unchanged; see
+ * {@link
org.apache.hadoop.security.authentication.server.AuthenticationFilter}.
+ * <p>
+ * This interface exists so that a SignerSecretProvider need not name a servlet
+ * type. Implementations of it that are backed by a ServletContext do, but the
+ * providers themselves stay independent of the servlet API and so of which
+ * servlet namespace - javax or jakarta - the container provides.
+ */
[email protected]
[email protected]
Review Comment:
Minor: the `@deprecated` text on `SignerSecretProvider.init` tells
implementors to override `initialize`, which takes this `@Private`/`@Unstable`
type. `SignerSecretProvider` is itself `@Private`, so this is consistent. But
`signer.secret.provider` accepts a class name, so external providers do exist
in practice, and the deprecation is effectively advice for them. Consider
`@LimitedPrivate`/`@Evolving` for the pair, or wording the deprecation note so
it doesn't promise more than the annotations do.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SignerSecretProvider.java:
##########
@@ -24,20 +24,77 @@
* do more complicated things in the backend.
* See the RolloverSignerSecretProvider class for an implementation that
* supports rolling over the secret at a regular interval.
+ * <p>
+ * Implementations should override
+ * {@link #initialize(Properties, SecretProviderContext, long)}, which names no
+ * servlet type. The older {@link #init(Properties, ServletContext, long)} is
+ * still the entry point callers use and is still honoured when an
+ * implementation overrides it, so providers written against it keep working
+ * unchanged; it is deprecated and will be removed with the move to the jakarta
+ * servlet namespace.
+ * <p>
+ * One consequence for code that reflects over these classes: the providers
+ * shipped here override
+ * {@link #initialize(Properties, SecretProviderContext, long)} rather than
+ * {@link #init(Properties, ServletContext, long)}, so init is no longer among
+ * their declared methods. Calls and overrides are unaffected - init is
+ * inherited from this class and resolves as it always did - but
+ * Class#getDeclaredMethod("init", ...) on a subclass such as
+ * {@link FileSignerSecretProvider} now raises NoSuchMethodException where it
+ * used to succeed. Class#getMethod, which searches superclasses, still finds
+ * it.
*/
@InterfaceStability.Unstable
@InterfaceAudience.Private
public abstract class SignerSecretProvider {
/**
- * Initialize the SignerSecretProvider
+ * Initialize the SignerSecretProvider.
+ * <p>
+ * The default implementation adapts the ServletContext to a
+ * {@link SecretProviderContext} and calls
+ * {@link #initialize(Properties, SecretProviderContext, long)}, so a
provider
+ * that overrides only that method is initialized correctly through this
+ * entry point. A provider that overrides this method instead is called
+ * directly, as before.
+ *
* @param config configuration properties
* @param servletContext servlet context
* @param tokenValidity The amount of time a token is valid for
* @throws Exception thrown if an error occurred
+ * @deprecated override
+ * {@link #initialize(Properties, SecretProviderContext, long)},
+ * which does not name a servlet type and so is unaffected by
+ * which servlet namespace the container provides.
*/
- public abstract void init(Properties config, ServletContext servletContext,
- long tokenValidity) throws Exception;
+ @Deprecated
+ public void init(Properties config, ServletContext servletContext,
Review Comment:
`init` is now `@Deprecated`, but every in-tree caller still uses it:
`AuthenticationFilter.constructSecretProvider` (5 calls) plus around 20 test
call sites. Each one now produces a new javac deprecation warning, which
Yetus's javac check is likely to flag against trunk. It also means nothing in
the tree calls `initialize` directly. Could `AuthenticationFilter` call
`initialize(config, ServletSecretProviderContext.of(ctx), validity)` for
providers that override `initialize`? Or, if the deprecation is mainly to steer
implementors, the deprecation could be deferred until the callers move.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/JWTRedirectAuthenticationHandler.java:
##########
@@ -125,7 +126,15 @@ public void init(Properties config) throws
ServletException {
throw new ServletException(
"Public key for signature validation must be provisioned.");
}
- publicKey = CertificateUtil.parseRSAPublicKey(pemPublicKey);
+ try {
+ publicKey = CertificateUtil.toRSAPublicKey(pemPublicKey);
+ } catch (CertificateException ce) {
+ // Report the exception toRSAPublicKey wrapped, not the wrapper, as
+ // CertificateUtil#parseRSAPublicKey does: the cause of this
+ // ServletException stays the CertificateException the parse raised.
+ Throwable cause = ce.getCause() == null ? ce : ce.getCause();
Review Comment:
This duplicates the unwrap-and-rewrap logic in
`CertificateUtil.parseRSAPublicKey`, including the null-cause branch that can't
happen. If the wrapping in `toRSAPublicKey` changes later, the two copies could
drift apart. See the comment on `CertificateUtil`: a shared helper would avoid
that.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ServletSecretProviderContext.java:
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.servlet.ServletContext;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.classification.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Presents a ServletContext to a {@link SignerSecretProvider} as a
+ * {@link SecretProviderContext}.
+ * <p>
+ * Reads and writes go straight through to the ServletContext, so an object a
+ * provider shares this way - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} - is still a ServletContext attribute under
+ * the same name, and is still found there by everything that looks for it,
+ * including DelegationTokenAuthenticationFilter in hadoop-common.
+ * <p>
+ * This class is the one place in the provider hierarchy that names a servlet
+ * type, and it goes away with the deprecated
+ * {@link SignerSecretProvider#init(java.util.Properties, ServletContext,
long)}
+ * it exists to support.
+ */
[email protected]
[email protected]
+final class ServletSecretProviderContext implements SecretProviderContext {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ServletSecretProviderContext.class);
+
+ private final ServletContext servletContext;
+
+ private ServletSecretProviderContext(ServletContext servletContext) {
+ this.servletContext = servletContext;
+ }
+
+ /**
+ * Returns a context backed by the given ServletContext, or one backed by a
+ * map of its own when there is no ServletContext.
+ * <p>
+ * Most callers of the deprecated init pass null - only AuthenticationFilter
+ * ever passes a real context - and a provider that does not use the store is
+ * unaffected either way. Handing back a store rather than null keeps a
+ * provider that does use it from having to check.
+ *
+ * @param servletContext the servlet context, or null
+ * @return an attribute store, never null
+ */
+ static SecretProviderContext of(ServletContext servletContext) {
+ return servletContext == null
+ ? new MapSecretProviderContext()
Review Comment:
I see the javadoc below calls this out deliberately, but it is a real
behaviour change for `ZKSignerSecretProvider`. On trunk, `init(config, null,
validity)` failed immediately (`servletContext.getAttribute` NPE). Now it
silently builds its own `CuratorFramework` client that nothing else can find
under `ZOOKEEPER_SIGNER_SECRET_PROVIDER_CURATOR_CLIENT_ATTRIBUTE`, so a
misconfigured caller gets an extra ZK session and possibly split secret/ACL
config, with only a WARN line.
Would it be safer to keep failing fast for a null context, e.g. have
`MapSecretProviderContext` (or `ZKSignerSecretProvider`) throw rather than warn
when asked to store the shared client? Providers that never touch the store
would be unaffected either way.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/CertificateUtil.java:
##########
@@ -57,7 +86,7 @@ public static RSAPublicKey parseRSAPublicKey(String pem)
throws ServletException
} else {
message = "CertificateException - PEM may be corrupt";
}
- throw new ServletException(message, ce);
+ throw new CertificateException(message, ce);
}
return (RSAPublicKey) key;
Review Comment:
This was already the case, but since `toRSAPublicKey` is new API documented
as reporting parse failures via `CertificateException`: a valid non-RSA
certificate (EC/DSA) gets through `generateCertificate` and then throws an
unchecked `ClassCastException` here. Suggest:
```java
if (!(key instanceof RSAPublicKey)) {
throw new CertificateException("Public key is not an RSA key: " +
key.getAlgorithm());
}
return (RSAPublicKey) key;
```
##########
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestCertificateUtil.java:
##########
@@ -87,12 +89,62 @@ public void testValidPEM() throws Exception {
+
"9aTyR+HGHCfvwoCegc9rAVw/DLaRriSO/jnEXzYK6XLVKH+hx5UXrJ7Oyc7JjZUc3g9kCWORThCX"
+ "Mzc1xA==";
try {
- RSAPublicKey pk = CertificateUtil.parseRSAPublicKey(pem);
+ RSAPublicKey pk = CertificateUtil.toRSAPublicKey(pem);
assertNotNull(pk);
assertEquals("RSA", pk.getAlgorithm());
+ } catch (CertificateException se) {
+ fail("Should not have thrown CertificateException");
+ }
+ }
+
+ /**
+ * The deprecated entry point still reports a ServletException, so callers
+ * written against it keep compiling and keep catching what they caught
+ * before.
+ */
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedEntryPointStillThrowsServletException() {
+ String pem = "not a certificate";
+ try {
+ CertificateUtil.parseRSAPublicKey(pem);
+ fail("Should not have thrown ServletException");
Review Comment:
The failure message reads the wrong way round: this `fail` runs when
`ServletException` was *not* thrown, so it should be something like `"Should
have thrown ServletException"`. Same at line 128. (The "Should not have thrown
CertificateException" messages at 51/72/96 look inherited, but they're touched
by this PR, so they'd be worth fixing too.)
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ServletSecretProviderContext.java:
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.servlet.ServletContext;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.classification.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Presents a ServletContext to a {@link SignerSecretProvider} as a
+ * {@link SecretProviderContext}.
+ * <p>
+ * Reads and writes go straight through to the ServletContext, so an object a
+ * provider shares this way - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} - is still a ServletContext attribute under
+ * the same name, and is still found there by everything that looks for it,
+ * including DelegationTokenAuthenticationFilter in hadoop-common.
+ * <p>
+ * This class is the one place in the provider hierarchy that names a servlet
+ * type, and it goes away with the deprecated
+ * {@link SignerSecretProvider#init(java.util.Properties, ServletContext,
long)}
+ * it exists to support.
+ */
[email protected]
[email protected]
+final class ServletSecretProviderContext implements SecretProviderContext {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ServletSecretProviderContext.class);
+
+ private final ServletContext servletContext;
+
+ private ServletSecretProviderContext(ServletContext servletContext) {
+ this.servletContext = servletContext;
+ }
+
+ /**
+ * Returns a context backed by the given ServletContext, or one backed by a
+ * map of its own when there is no ServletContext.
+ * <p>
+ * Most callers of the deprecated init pass null - only AuthenticationFilter
+ * ever passes a real context - and a provider that does not use the store is
+ * unaffected either way. Handing back a store rather than null keeps a
+ * provider that does use it from having to check.
+ *
+ * @param servletContext the servlet context, or null
+ * @return an attribute store, never null
+ */
+ static SecretProviderContext of(ServletContext servletContext) {
+ return servletContext == null
+ ? new MapSecretProviderContext()
+ : new ServletSecretProviderContext(servletContext);
+ }
+
+ @Override
+ public Object getAttribute(String name) {
+ return servletContext.getAttribute(name);
+ }
+
+ @Override
+ public void setAttribute(String name, Object value) {
+ servletContext.setAttribute(name, value);
+ }
+
+ /**
+ * The store used when there is no ServletContext to write through to.
+ * Attributes live as long as the provider does and are seen by nothing else,
+ * which is what passing a null ServletContext already meant.
+ */
+ @VisibleForTesting
+ static final class MapSecretProviderContext
+ implements SecretProviderContext {
+
+ private final Map<String, Object> attributes = new HashMap<>();
+
+ @Override
+ public Object getAttribute(String name) {
+ return attributes.get(name);
+ }
+
+ /**
+ * Stores the attribute, and says so.
+ * <p>
+ * A provider only writes here to share an object with the rest of the
+ * application - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} is the one case in this tree. There is
+ * nothing to share it with: this store is private to the provider, so a
+ * second provider in the same JVM builds a second client rather than
+ * finding this one. Before there was a store at all a null ServletContext
+ * threw, which was at least loud; this keeps it visible.
+ */
+ @Override
+ public void setAttribute(String name, Object value) {
+ LOG.warn("SignerSecretProvider was initialized without a ServletContext,"
+ + " so the attribute {} is being stored in a map private to this"
+ + " provider. Anything that expects to share it - including another"
+ + " provider in this JVM - will not find it there.", name);
+ attributes.put(name, value);
Review Comment:
Nit: `ServletContext.setAttribute(name, null)` removes the attribute,
whereas this stores a null value, so the two stores behave differently for a
provider that clears an attribute. Easy to match with `if (value == null)
attributes.remove(name); else attributes.put(name, value);`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
