Skip to content

Fix 1799 and Spring 33450 #2062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,23 @@ public void testPublish() {
assertEquals("Log4j", value);
}

@Test
@ResourceLock(Resources.SYSTEM_PROPERTIES)
public void testBadPropertysource() {
final String key1 = "testKey";
System.getProperties().put(key1, "test");
final PropertiesUtil util = new PropertiesUtil(new Properties());
ErrorPropertySource source = new ErrorPropertySource();
util.addPropertySource(source);
try {
assertEquals("test", util.getStringProperty(key1));
assertTrue(source.exceptionThrown);
} finally {
util.removePropertySource(source);
System.getProperties().remove(key1);
}
}

private static final String[][] data = {
{null, "org.apache.logging.log4j.level"},
{null, "Log4jAnotherProperty"},
Expand Down Expand Up @@ -184,4 +201,25 @@ public void testLog4jProperty() {
final PropertiesUtil util = new PropertiesUtil(props);
assertNull(util.getStringProperty(correct));
}

private class ErrorPropertySource implements PropertySource {
public boolean exceptionThrown = false;

@Override
public int getPriority() {
return Integer.MIN_VALUE;
}

@Override
public String getProperty(String key) {
exceptionThrown = true;
throw new InstantiationError("Test");
}

@Override
public boolean containsProperty(String key) {
exceptionThrown = true;
throw new InstantiationError("Test");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,13 @@ public void addPropertySource(final PropertySource propertySource) {
}
}

@Override
public void removePropertySource(final PropertySource propertySource) {
if (environment != null) {
environment.removePropertySource(propertySource);
}
}

/**
* Returns {@code true} if the specified property is defined, regardless of its value (it may not have a value).
*
Expand Down Expand Up @@ -683,6 +690,11 @@ public void addPropertySource(final PropertySource propertySource) {
sources.add(propertySource);
}

@Override
public void removePropertySource(final PropertySource propertySource) {
sources.remove(propertySource);
}

private void reload() {
literal.clear();
sources.forEach((s) -> {
Expand All @@ -700,14 +712,14 @@ private void reload() {
sources.forEach(source -> {
if (source instanceof ContextAwarePropertySource) {
final ContextAwarePropertySource src = Cast.cast(source);
if (src.containsProperty(contextName, contextKey)) {
if (sourceContainsProperty(src, contextName, contextKey)) {
literal.putIfAbsent(key, src.getProperty(contextName, contextKey));
}
}
});
}
sources.forEach(source -> {
if (source.containsProperty(contextKey)) {
if (sourceContainsProperty(source, contextKey)) {
literal.putIfAbsent(key, source.getProperty(contextKey));
}
});
Expand All @@ -727,7 +739,7 @@ public String getStringProperty(final String key) {
while (source != null) {
if (source instanceof ContextAwarePropertySource) {
final ContextAwarePropertySource src = Cast.cast(source);
result = src.getProperty(contextName, contextKey);
result = sourceGetProperty(src, contextName, contextKey);
}
if (result != null) {
return result;
Expand All @@ -737,7 +749,7 @@ public String getStringProperty(final String key) {
}
PropertySource source = sources.first();
while (source != null) {
result = source.getProperty(contextKey);
result = sourceGetProperty(source, contextKey);
if (result != null) {
return result;
}
Expand All @@ -746,6 +758,22 @@ public String getStringProperty(final String key) {
return result;
}

private String sourceGetProperty(ContextAwarePropertySource source, String contextName, String key) {
try {
return source.getProperty(contextName, key);
} catch (Throwable ex) {
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we better dump ex using the StatusLogger or something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The issue that was reported simply causes error messages to be logged. The application is shutting down so an error during shutdown is irritating. Changing that to a warning doesn't really help much.

}
}

private String sourceGetProperty(PropertySource source, String key) {
try {
return source.getProperty(key);
} catch (Throwable ex) {
return null;
}
}

@Override
public boolean hasProperty(final String key) {
if (literal.containsKey(key)) {
Expand All @@ -758,7 +786,7 @@ public boolean hasProperty(final String key) {
while (source != null) {
if (source instanceof ContextAwarePropertySource) {
final ContextAwarePropertySource src = Cast.cast(source);
if (src.containsProperty(contextName, contextKey)) {
if (sourceContainsProperty(src, contextName, contextKey)) {
return true;
}
}
Expand All @@ -769,9 +797,9 @@ public boolean hasProperty(final String key) {
while (source != null) {
if (source instanceof ContextAwarePropertySource) {
final ContextAwarePropertySource src = Cast.cast(source);
if (src.containsProperty(contextName, contextKey)
if (sourceContainsProperty(src, contextName, contextKey)
|| (!contextName.equals(PropertySource.SYSTEM_CONTEXT)
&& src.containsProperty(PropertySource.SYSTEM_CONTEXT, contextKey))) {
&& sourceContainsProperty(src, PropertySource.SYSTEM_CONTEXT, contextKey))) {
return true;
}
} else {
Expand All @@ -784,6 +812,22 @@ public boolean hasProperty(final String key) {
return false;
}

private boolean sourceContainsProperty(ContextAwarePropertySource source, String contextName, String key) {
try {
return source.containsProperty(contextName, key);
} catch (Throwable ex) {
return false;
}
}

private boolean sourceContainsProperty(PropertySource source, String key) {
try {
return source.containsProperty(key);
} catch (Throwable ex) {
return false;
}
}

private String getContextKey(final String key) {
String keyToCheck = key;
if (keyToCheck.startsWith(PropertySource.PREFIX)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ public interface PropertyEnvironment {
*/
void addPropertySource(PropertySource propertySource);

/**
* Allows a PropertySource that was added to be removed.
* @param propertySource the PropertySource to remove.
*/
void removePropertySource(PropertySource propertySource);

/**
* Returns {@code true} if the specified property is defined, regardless of its value (it may not have a value).
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to you 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.
*/
package org.apache.logging.log4j.core;

import static org.junit.jupiter.api.Assertions.assertTrue;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.impl.Log4jContextFactory;
import org.apache.logging.log4j.core.impl.internal.InternalLoggerContext;
import org.apache.logging.log4j.core.util.DefaultShutdownCallbackRegistry;
import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry;
import org.apache.logging.log4j.spi.LoggerContextFactory;
import org.junit.jupiter.api.Test;

/**
* Validate Logging after Shutdown.
*/
public class LoggerContextTest {

@Test
public void shutdownTest() {
LoggerContextFactory contextFactory = LogManager.getFactory();
assertTrue(contextFactory instanceof Log4jContextFactory);
Log4jContextFactory factory = (Log4jContextFactory) contextFactory;
ShutdownCallbackRegistry registry = factory.getShutdownCallbackRegistry();
assertTrue(registry instanceof DefaultShutdownCallbackRegistry);
((DefaultShutdownCallbackRegistry) registry).start();
((DefaultShutdownCallbackRegistry) registry).stop();
LoggerContext loggerContext = factory.getContext(LoggerContextTest.class.getName(), null, null, false);
assertTrue(loggerContext instanceof InternalLoggerContext);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.hamcrest.Matchers.hasKey;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.*;

import java.util.Map;
Expand Down Expand Up @@ -48,7 +49,7 @@ public void testMissingRootLogger(final LoggerContext ctx) throws Exception {
assertNotNull(map, "Appenders not null");
assertThat("There should only be two appenders", map, hasSize(2));
assertThat(map, hasKey("List"));
assertThat(map, hasKey("DefaultConsole-2"));
assertThat(map, hasKey(startsWith("DefaultConsole-")));

final Map<String, LoggerConfig> loggerMap = config.getLoggers();
assertNotNull(loggerMap, "loggerMap not null");
Expand All @@ -67,7 +68,8 @@ public void testMissingRootLogger(final LoggerContext ctx) throws Exception {
final Map<String, Appender> rootAppenders = root.getAppenders();
assertThat("The root logger should only have one appender", rootAppenders, hasSize(1));
// root only has Console appender!
assertThat("The root appender should be a ConsoleAppender", rootAppenders, hasKey("DefaultConsole-2"));
assertThat(
"The root appender should be a ConsoleAppender", rootAppenders, hasKey(startsWith("DefaultConsole-")));
assertEquals(Level.ERROR, root.getLevel());
}
}
11 changes: 11 additions & 0 deletions log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ protected Logger(final LoggerContext context, final String name, final MessageFa
privateConfig = new PrivateConfig(context.getConfiguration(), this);
}

/**
* This is used to construct an InternalLoggerContext, which makes SimpleLoggerContext conmpatible with core.
* @param context the InternalLoggerContext.
* @param name the Logger name.
*/
protected Logger(final LoggerContext context, final String name) {
super(name);
this.context = context;
privateConfig = null;
}

/**
* This method is only used for 1.x compatibility. Returns the parent of this Logger. If it doesn't already exist
* return a temporary Logger.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ public class LoggerContext extends AbstractLifeCycle

private final Lock configLock = new ReentrantLock();

/**
* Constructor used to create an InternalLoggerContext.
*/
protected LoggerContext() {
setStarted();
instanceFactory = null;
this.nullConfiguration = null;
}

/**
* Constructor taking only a name.
*
Expand Down Expand Up @@ -428,6 +437,7 @@ public boolean stop(final long timeout, final TimeUnit timeUnit) {
}

this.setStopping();
String name = getName();
try {
Server.unregisterLoggerContext(getName()); // LOG4J2-406, LOG4J2-500
} catch (final LinkageError | Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory;
import org.apache.logging.log4j.core.impl.Log4jPropertyKey;
import org.apache.logging.log4j.core.util.AuthorizationProvider;
import org.apache.logging.log4j.plugins.Namespace;
import org.apache.logging.log4j.plugins.di.ConfigurableInstanceFactory;
import org.apache.logging.log4j.plugins.di.Key;
import org.apache.logging.log4j.plugins.model.PluginNamespace;
import org.apache.logging.log4j.status.StatusLogger;
import org.apache.logging.log4j.util.LoaderUtil;
import org.apache.logging.log4j.util.PropertiesUtil;
import org.apache.logging.log4j.util.PropertyEnvironment;
import org.apache.logging.log4j.util.PropertyKey;

/**
Expand Down Expand Up @@ -118,6 +121,15 @@ protected boolean isActive() {
return true;
}

/**
* Required for Spring Boot.
* @param props PropertiesUtil.
* @return the AuthorizationProvider, if any.
*/
public static AuthorizationProvider authorizationProvider(final PropertiesUtil props) {
return AuthorizationProvider.getAuthorizationProvider((PropertyEnvironment) props);
}
Comment on lines +129 to +131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not really sure if ConfigurationFactory is the right place to provide the AuthorizationProvider. Does it really need to be placed here? I mean, the call site can themselves issue AuthorizationProvider.getAuthorizationProvider() too, ain't so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the method should not be duplicated.

Also, in order to make this PR back-portable to 2.x (it is a requirement, isn't it?), I would use LoggerContext as parameter.

Unless I am mistaken, all HTTP/HTTPS accesses are performed after a LoggerContext is created and the context is passed to the ConfigurationFactory as a parameter (since 2.7). This change would allow 3.x to configure separate credentials for separate web applications.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authorization provider is always created based on properties, partly because it can be used to authenticate with a remote source that provides your logging config.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvz,

Yes, but it could use a different PropertyEnvironment for each LoggerContext.

Anyway the reason @rgoers "introduced" this method in 3.x is that it was already present in 2.x, but disappeared from 3.x or was never ported to main.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppkarwasz Correct. It is NOT a new method. Matt removed it a few weeks before I made this fix.


public abstract Configuration getConfiguration(final LoggerContext loggerContext, ConfigurationSource source);

/**
Expand Down
Loading