You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "cziegeler (via GitHub)" <gi...@apache.org> on 2023/06/04 08:26:48 UTC
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler opened a new pull request, #11: Issues/sling 11741
cziegeler opened a new pull request, #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11
(no comment)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] sonarcloud[bot] commented on pull request #11: Issues/sling 11741
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1576040926
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [12 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list) [73.8% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] bdelacretaz commented on a diff in pull request #11: Issues/sling 11741
Posted by "bdelacretaz (via GitHub)" <gi...@apache.org>.
bdelacretaz commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1329745936
##########
src/main/java/org/apache/sling/jcr/base/internal/AllowListFragment.java:
##########
@@ -32,59 +32,58 @@
import static java.util.Arrays.asList;
@ObjectClassDefinition(
- name = "Apache Sling Login Admin Whitelist Configuration Fragment",
- description = "Whitelist configuration fragments contribute a list of whitelisted bundle symbolic " +
- "names to the Login Admin Whitelist. This allows for modularisation of the whitelist."
+ name = "Apache Sling Login Admin Allow List Configuration Fragment",
+ description = "Allow list configuration fragments contribute a list of allow listed bundle symbolic " +
Review Comment:
I think rewording the description would make it easier to understand, just replacing "white list" with "allow list" makes it hard to read IMHO, I suggest:
> This list of Bundle Symbolic Names is added to the list of bundles which are allowed to use Administrative Login. The full list is built, in a modular way, out of all such configuration fragments.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] sonarcloud[bot] commented on pull request #11: Issues/sling 11741
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1736216214
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list) [82.7% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337172793
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
Review Comment:
Please suggest a better wording :)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337238395
##########
src/main/java/org/apache/sling/jcr/base/internal/LegacyFragment.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.converter.Converters;
+
+import java.util.Map;
+
+/**
+ * Legacy fragment configuration. Use {@link AllowListFragment} instead.
+ */
+@Component(
+ configurationPid = LegacyFragment.LEGACY_FACTORY_PID,
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = LegacyFragment.class
+)
+public class LegacyFragment {
+
+ public static final String LEGACY_FACTORY_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+
+ private static final String LEGACY_NAME = "whitelist.name";
+ private static final String LEGACY_BUNDLES = "whitelist.bundles";
+
+ private final AllowListFragment fragment;
+
+ private final LoginAdminAllowList allowList;
+
+ @Activate
+ public LegacyFragment(final @Reference LoginAdminAllowList allowList, final Map<String, Object> config) {
+ LoginAdminAllowList.LOG.warn("Using deprecated factory configuration '{}'. " +
+ "Update your configuration to use configuration '{}' instead.",
+ LEGACY_FACTORY_PID, AllowListFragment.FACTORY_PID);
+ this.allowList = allowList;
+ final String name = Converters.standardConverter().convert(config.get(LEGACY_NAME)).to(String.class);
+ final String[] bundles = Converters.standardConverter().convert(config.get(LEGACY_BUNDLES)).to(String[].class);
+ this.fragment = new AllowListFragment(name, bundles);
+ this.allowList.bindAllowListFragment(fragment);
Review Comment:
I din't mean to register the service manually. I meant to have different implementations of the `AllowListFragment`, both DS components registered as services.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1341023515
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
Review Comment:
Fine with me.
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
+ }
+ this.bypassAllowList = bypass;
+ if (this.bypassAllowList) {
LOG.info("bypassAllowlist=true, allowlisted BSNs=<ALL>");
LOG.warn("All bundles are allowed to use loginAdministrative due to the 'allowlist.bypass' " +
Review Comment:
Fine with me.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1740452497
@cziegeler if you're happy with the current state, feel free to merge!
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1219599388
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
+
+ static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist";
+ static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID;
+ private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>();
+ static {
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass");
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp");
+ }
+
+ private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+ private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID;
+ private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>();
+
+ static {
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name");
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles");
+ }
+
+ protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ private final List<Updater> configurationUpdaterList = new ArrayList<>();
+
+ private final ConfigurationAdmin configurationAdmin;
+
+ @Activate
+ public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) {
+ this.configurationAdmin = configurationAdmin;
+ configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE));
+ configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE));
+
+ configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps());
+ }
+
+ @Override
+ public void configurationEvent(final ConfigurationEvent event) {
+ if ( event.getType() == ConfigurationEvent.CM_UPDATED ) {
+ configurationUpdaterList.forEach(configurationUpdater -> {
+ configurationUpdater.updateProps(event);
+ });
+ }
+ }
+
+ /**
+ * Encode the value for the ldap filter: \, *, (, and ) should be escaped.
+ */
+ private static String encode(final String value) {
+ return value.replace("\\", "\\\\")
+ .replace("*", "\\*")
+ .replace("(", "\\(")
+ .replace(")", "\\)");
+ }
+
+ protected abstract class Updater {
+
+ protected final String oldPid;
+ protected final String newPid;
+ protected final Map<String, String> propsToReplace;
+
+ public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ this.oldPid = oldPid;
+ this.newPid = newPid;
+ this.propsToReplace = propsToReplace;
+ }
+
+ protected abstract void updateProps(ConfigurationEvent event);
+
+ protected abstract void updateProps();
+
+ protected abstract Configuration createConfiguration(String oldPid) throws IOException;
+
+ /**
+ * Update a configuration
+ */
+ protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) {
+ final Dictionary<String, Object> sourceProps = sourceConfig.getProperties();
+ final Dictionary<String, Object> targetProps = new Hashtable<>();
+ for(final String name : Collections.list(sourceProps.keys())) {
+ targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name));
+ }
+ try {
+ final Configuration cfg = this.createConfiguration(sourceConfig.getPid());
+ if (cfg==null) return;
+ cfg.update(targetProps);
+ sourceConfig.delete();
+ logger.info("Updated configuration with PID {} to new configuration with PID {}. "+
+ "Please see https://sling.apache.org/documentation/the-sling-engine/service-authentication.html for more information.",
+ sourceConfig.getPid(), cfg.getPid());
+ } catch (final IOException e) {
+ logger.warn("Failed to update configuration with PID {}", sourceConfig.getPid(), e);
+ }
+ }
+ }
+
+ private class PidConfigurationUpdater extends Updater {
+
+ public PidConfigurationUpdater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ super(oldPid, newPid, propsToReplace);
+ }
+
+ @Override
+ protected void updateProps(final ConfigurationEvent event) {
+ if (this.oldPid.equals(event.getPid())) {
Review Comment:
@sagarmiglani Thanks, ok, I updated the code to leave the new configuration as-is if it already exists. changed log messages to warn
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1334302571
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -47,34 +45,28 @@
* use the loginAdministrative method.
*/
@Component(service = LoginAdminAllowList.class,
- configurationPid = LoginAdminAllowList.PID,
- reference = {
- // ConfigurationUpdater is a required dependency to make sure that configurations are
- // updated before this component is activated
- @Reference(
- name = "ConfigurationUpdater",
- service = ConfigurationUpdater.class,
- cardinality = ReferenceCardinality.MANDATORY
- )
- }
-)
-@Designate(
- ocd = LoginAdminAllowListConfiguration.class
+ configurationPid = {LoginAdminAllowList.PID, LoginAdminAllowList.LEGACY_PID}
Review Comment:
I think the two PIDs should be swapped around. The later PID has higher precedence.
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -83,23 +75,22 @@ public class LoginAdminAllowList {
cardinality = ReferenceCardinality.MULTIPLE,
policy = ReferencePolicy.DYNAMIC,
policyOption = ReferencePolicyOption.GREEDY
- ) @SuppressWarnings("unused")
+ )
void bindAllowListFragment(AllowListFragment fragment) {
allowListFragments.add(fragment);
LOG.info("AllowListFragment added '{}'", fragment);
}
- @SuppressWarnings("unused")
void unbindAllowListFragment(AllowListFragment fragment) {
allowListFragments.remove(fragment);
LOG.info("AllowListFragment removed '{}'", fragment);
}
- @Activate @Modified @SuppressWarnings("unused")
- void configure(LoginAdminAllowListConfiguration configuration, Map<String, Object> properties) {
- this.config = new ConfigurationState(configuration);
- ensureBackwardsCompatibility(properties, PROP_WHITELIST_BUNDLES_DEFAULT);
- ensureBackwardsCompatibility(properties, PROP_WHITELIST_BUNDLES_ADDITIONAL);
+ @Activate @Modified
+ void configure(final LoginAdminAllowListConfiguration configuration, final Map<String, Object> properties) {
Review Comment:
What about using a second annotation, without metatype, like you did with the [`VanityPathConfigurer.DeprecatedVanityConfig`](https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/a24a17467b334f9076f91c2dccd2491dfc9ae96b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java#L221)?
##########
src/main/java/org/apache/sling/jcr/base/internal/LegacyFragment.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.converter.Converters;
+
+import java.util.Map;
+
+/**
+ * Legacy fragment configuration. Use {@link AllowListFragment} instead.
+ */
+@Component(
+ configurationPid = LegacyFragment.LEGACY_FACTORY_PID,
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = LegacyFragment.class
+)
+public class LegacyFragment {
+
+ public static final String LEGACY_FACTORY_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+
+ private static final String LEGACY_NAME = "whitelist.name";
+ private static final String LEGACY_BUNDLES = "whitelist.bundles";
+
+ private final AllowListFragment fragment;
+
+ private final LoginAdminAllowList allowList;
+
+ @Activate
+ public LegacyFragment(final @Reference LoginAdminAllowList allowList, final Map<String, Object> config) {
+ LoginAdminAllowList.LOG.warn("Using deprecated factory configuration '{}'. " +
+ "Update your configuration to use configuration '{}' instead.",
+ LEGACY_FACTORY_PID, AllowListFragment.FACTORY_PID);
+ this.allowList = allowList;
+ final String name = Converters.standardConverter().convert(config.get(LEGACY_NAME)).to(String.class);
+ final String[] bundles = Converters.standardConverter().convert(config.get(LEGACY_BUNDLES)).to(String[].class);
+ this.fragment = new AllowListFragment(name, bundles);
+ this.allowList.bindAllowListFragment(fragment);
Review Comment:
As an alternative to manual binding/unbinding, we could extend `LegacyFragment` from `AllowListFragment`, registering it as an `AllowListFragment` service. The configuration could be injected via annotation object, but without metatype, and the values passed to the `AllowListFragment(String name, String[] bundles)` constructor. I think this would simplify the code further.
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
Review Comment:
IMHO it could be clearer that the second property name is not deprecated. Maybe reword to something like "... and its non-deprecated equivalent '{}' are set.". WDYT?
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
Review Comment:
Should we deprecate the regexp for removal? And only leave support for "whitelist.bundles.regexp"?
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
+ }
+ this.bypassAllowList = bypass;
+ if (this.bypassAllowList) {
LOG.info("bypassAllowlist=true, allowlisted BSNs=<ALL>");
LOG.warn("All bundles are allowed to use loginAdministrative due to the 'allowlist.bypass' " +
Review Comment:
Deprecate for removal? Or is it too useful for development?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337169650
##########
src/main/java/org/apache/sling/jcr/base/internal/LegacyFragment.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.converter.Converters;
+
+import java.util.Map;
+
+/**
+ * Legacy fragment configuration. Use {@link AllowListFragment} instead.
+ */
+@Component(
+ configurationPid = LegacyFragment.LEGACY_FACTORY_PID,
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = LegacyFragment.class
+)
+public class LegacyFragment {
+
+ public static final String LEGACY_FACTORY_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+
+ private static final String LEGACY_NAME = "whitelist.name";
+ private static final String LEGACY_BUNDLES = "whitelist.bundles";
+
+ private final AllowListFragment fragment;
+
+ private final LoginAdminAllowList allowList;
+
+ @Activate
+ public LegacyFragment(final @Reference LoginAdminAllowList allowList, final Map<String, Object> config) {
+ LoginAdminAllowList.LOG.warn("Using deprecated factory configuration '{}'. " +
+ "Update your configuration to use configuration '{}' instead.",
+ LEGACY_FACTORY_PID, AllowListFragment.FACTORY_PID);
+ this.allowList = allowList;
+ final String name = Converters.standardConverter().convert(config.get(LEGACY_NAME)).to(String.class);
+ final String[] bundles = Converters.standardConverter().convert(config.get(LEGACY_BUNDLES)).to(String[].class);
+ this.fragment = new AllowListFragment(name, bundles);
+ this.allowList.bindAllowListFragment(fragment);
Review Comment:
Yes, I decided against this as it is an anti-pattern to register services within an activate method.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] sagarmiglani commented on a diff in pull request #11: Issues/sling 11741
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1218986013
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
+
+ static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist";
+ static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID;
+ private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>();
+ static {
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass");
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp");
+ }
+
+ private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+ private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID;
+ private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>();
+
+ static {
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name");
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles");
+ }
+
+ protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ private final List<Updater> configurationUpdaterList = new ArrayList<>();
+
+ private final ConfigurationAdmin configurationAdmin;
+
+ @Activate
+ public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) {
+ this.configurationAdmin = configurationAdmin;
+ configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE));
+ configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE));
+
+ configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps());
+ }
+
+ @Override
+ public void configurationEvent(final ConfigurationEvent event) {
+ if ( event.getType() == ConfigurationEvent.CM_UPDATED ) {
+ configurationUpdaterList.forEach(configurationUpdater -> {
+ configurationUpdater.updateProps(event);
+ });
+ }
+ }
+
+ /**
+ * Encode the value for the ldap filter: \, *, (, and ) should be escaped.
+ */
+ private static String encode(final String value) {
+ return value.replace("\\", "\\\\")
+ .replace("*", "\\*")
+ .replace("(", "\\(")
+ .replace(")", "\\)");
+ }
+
+ protected abstract class Updater {
+
+ protected final String oldPid;
+ protected final String newPid;
+ protected final Map<String, String> propsToReplace;
+
+ public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ this.oldPid = oldPid;
+ this.newPid = newPid;
+ this.propsToReplace = propsToReplace;
+ }
+
+ protected abstract void updateProps(ConfigurationEvent event);
+
+ protected abstract void updateProps();
+
+ protected abstract Configuration createConfiguration(String oldPid) throws IOException;
+
+ /**
+ * Update a configuration
+ */
+ protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) {
+ final Dictionary<String, Object> sourceProps = sourceConfig.getProperties();
+ final Dictionary<String, Object> targetProps = new Hashtable<>();
+ for(final String name : Collections.list(sourceProps.keys())) {
+ targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name));
+ }
+ try {
+ final Configuration cfg = this.createConfiguration(sourceConfig.getPid());
+ if (cfg==null) return;
+ cfg.update(targetProps);
+ sourceConfig.delete();
+ logger.info("Updated configuration with PID {} to new configuration with PID {}. "+
+ "Please see https://sling.apache.org/documentation/the-sling-engine/service-authentication.html for more information.",
+ sourceConfig.getPid(), cfg.getPid());
+ } catch (final IOException e) {
+ logger.warn("Failed to update configuration with PID {}", sourceConfig.getPid(), e);
+ }
+ }
+ }
+
+ private class PidConfigurationUpdater extends Updater {
+
+ public PidConfigurationUpdater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ super(oldPid, newPid, propsToReplace);
+ }
+
+ @Override
+ protected void updateProps(final ConfigurationEvent event) {
+ if (this.oldPid.equals(event.getPid())) {
Review Comment:
Would it make sense to log a warning and instruct the user to use the new configuration? Also what if a user has both old and new configuration in place?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] sagarmiglani commented on a diff in pull request #11: Issues/sling 11741
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1219482581
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
+
+ static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist";
+ static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID;
+ private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>();
+ static {
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass");
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp");
+ }
+
+ private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+ private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID;
+ private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>();
+
+ static {
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name");
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles");
+ }
+
+ protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ private final List<Updater> configurationUpdaterList = new ArrayList<>();
+
+ private final ConfigurationAdmin configurationAdmin;
+
+ @Activate
+ public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) {
+ this.configurationAdmin = configurationAdmin;
+ configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE));
+ configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE));
+
+ configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps());
+ }
+
+ @Override
+ public void configurationEvent(final ConfigurationEvent event) {
+ if ( event.getType() == ConfigurationEvent.CM_UPDATED ) {
+ configurationUpdaterList.forEach(configurationUpdater -> {
+ configurationUpdater.updateProps(event);
+ });
+ }
+ }
+
+ /**
+ * Encode the value for the ldap filter: \, *, (, and ) should be escaped.
+ */
+ private static String encode(final String value) {
+ return value.replace("\\", "\\\\")
+ .replace("*", "\\*")
+ .replace("(", "\\(")
+ .replace(")", "\\)");
+ }
+
+ protected abstract class Updater {
+
+ protected final String oldPid;
+ protected final String newPid;
+ protected final Map<String, String> propsToReplace;
+
+ public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ this.oldPid = oldPid;
+ this.newPid = newPid;
+ this.propsToReplace = propsToReplace;
+ }
+
+ protected abstract void updateProps(ConfigurationEvent event);
+
+ protected abstract void updateProps();
+
+ protected abstract Configuration createConfiguration(String oldPid) throws IOException;
+
+ /**
+ * Update a configuration
+ */
+ protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) {
+ final Dictionary<String, Object> sourceProps = sourceConfig.getProperties();
+ final Dictionary<String, Object> targetProps = new Hashtable<>();
+ for(final String name : Collections.list(sourceProps.keys())) {
+ targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name));
+ }
+ try {
+ final Configuration cfg = this.createConfiguration(sourceConfig.getPid());
+ if (cfg==null) return;
+ cfg.update(targetProps);
+ sourceConfig.delete();
+ logger.info("Updated configuration with PID {} to new configuration with PID {}. "+
+ "Please see https://sling.apache.org/documentation/the-sling-engine/service-authentication.html for more information.",
+ sourceConfig.getPid(), cfg.getPid());
+ } catch (final IOException e) {
+ logger.warn("Failed to update configuration with PID {}", sourceConfig.getPid(), e);
+ }
+ }
+ }
+
+ private class PidConfigurationUpdater extends Updater {
+
+ public PidConfigurationUpdater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ super(oldPid, newPid, propsToReplace);
+ }
+
+ @Override
+ protected void updateProps(final ConfigurationEvent event) {
+ if (this.oldPid.equals(event.getPid())) {
Review Comment:
> Good points. We log the situation when the configuration is actually converted with a link to the docs. Do you think we need more?
I think link to the docs in enough, but logging warning instead of info seems to be a better option. "Info" messages can typically be disregarded as they provide general information.
> If the new configuration exists, it will be overwritten by the old one and then the old one will be deleted.
We could handle this differently and not touch the new configuration but just log and delete the old one?
While both solutions appear to be viable options, IMO, it makes slightly more sense not to touch the new configuration. This approach aligns with our goal of encouraging users to migrate to the newer configurations. What do you think?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1329754434
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,236 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
Review Comment:
We hope that we don't have more of those. So for now no need to have a generic solution.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler merged pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler merged PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1219277825
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
+
+ static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist";
+ static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID;
+ private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>();
+ static {
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass");
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp");
+ }
+
+ private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+ private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID;
+ private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>();
+
+ static {
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name");
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles");
+ }
+
+ protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ private final List<Updater> configurationUpdaterList = new ArrayList<>();
+
+ private final ConfigurationAdmin configurationAdmin;
+
+ @Activate
+ public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) {
+ this.configurationAdmin = configurationAdmin;
+ configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE));
+ configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE));
+
+ configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps());
+ }
+
+ @Override
+ public void configurationEvent(final ConfigurationEvent event) {
+ if ( event.getType() == ConfigurationEvent.CM_UPDATED ) {
+ configurationUpdaterList.forEach(configurationUpdater -> {
+ configurationUpdater.updateProps(event);
+ });
+ }
+ }
+
+ /**
+ * Encode the value for the ldap filter: \, *, (, and ) should be escaped.
+ */
+ private static String encode(final String value) {
+ return value.replace("\\", "\\\\")
+ .replace("*", "\\*")
+ .replace("(", "\\(")
+ .replace(")", "\\)");
+ }
+
+ protected abstract class Updater {
+
+ protected final String oldPid;
+ protected final String newPid;
+ protected final Map<String, String> propsToReplace;
+
+ public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ this.oldPid = oldPid;
+ this.newPid = newPid;
+ this.propsToReplace = propsToReplace;
+ }
+
+ protected abstract void updateProps(ConfigurationEvent event);
+
+ protected abstract void updateProps();
+
+ protected abstract Configuration createConfiguration(String oldPid) throws IOException;
+
+ /**
+ * Update a configuration
+ */
+ protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) {
+ final Dictionary<String, Object> sourceProps = sourceConfig.getProperties();
+ final Dictionary<String, Object> targetProps = new Hashtable<>();
+ for(final String name : Collections.list(sourceProps.keys())) {
+ targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name));
+ }
+ try {
+ final Configuration cfg = this.createConfiguration(sourceConfig.getPid());
+ if (cfg==null) return;
+ cfg.update(targetProps);
+ sourceConfig.delete();
+ logger.info("Updated configuration with PID {} to new configuration with PID {}. "+
+ "Please see https://sling.apache.org/documentation/the-sling-engine/service-authentication.html for more information.",
+ sourceConfig.getPid(), cfg.getPid());
+ } catch (final IOException e) {
+ logger.warn("Failed to update configuration with PID {}", sourceConfig.getPid(), e);
+ }
+ }
+ }
+
+ private class PidConfigurationUpdater extends Updater {
+
+ public PidConfigurationUpdater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ super(oldPid, newPid, propsToReplace);
+ }
+
+ @Override
+ protected void updateProps(final ConfigurationEvent event) {
+ if (this.oldPid.equals(event.getPid())) {
Review Comment:
Good points. We log the situation when the configuration is actually converted with a link to the docs. Do you think we need more?
If the new configuration exists, it will be overwritten by the old one and then the old one will be deleted.
We could handle this differently and not touch the new configuration but just log and delete the old one?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337236451
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -47,34 +45,28 @@
* use the loginAdministrative method.
*/
@Component(service = LoginAdminAllowList.class,
- configurationPid = LoginAdminAllowList.PID,
- reference = {
- // ConfigurationUpdater is a required dependency to make sure that configurations are
- // updated before this component is activated
- @Reference(
- name = "ConfigurationUpdater",
- service = ConfigurationUpdater.class,
- cardinality = ReferenceCardinality.MANDATORY
- )
- }
-)
-@Designate(
- ocd = LoginAdminAllowListConfiguration.class
+ configurationPid = {LoginAdminAllowList.PID, LoginAdminAllowList.LEGACY_PID}
Review Comment:
That's right, yes.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1338633803
##########
src/main/java/org/apache/sling/jcr/base/internal/LegacyFragment.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.converter.Converters;
+
+import java.util.Map;
+
+/**
+ * Legacy fragment configuration. Use {@link AllowListFragment} instead.
+ */
+@Component(
+ configurationPid = LegacyFragment.LEGACY_FACTORY_PID,
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = LegacyFragment.class
+)
+public class LegacyFragment {
+
+ public static final String LEGACY_FACTORY_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+
+ private static final String LEGACY_NAME = "whitelist.name";
+ private static final String LEGACY_BUNDLES = "whitelist.bundles";
+
+ private final AllowListFragment fragment;
+
+ private final LoginAdminAllowList allowList;
+
+ @Activate
+ public LegacyFragment(final @Reference LoginAdminAllowList allowList, final Map<String, Object> config) {
+ LoginAdminAllowList.LOG.warn("Using deprecated factory configuration '{}'. " +
+ "Update your configuration to use configuration '{}' instead.",
+ LEGACY_FACTORY_PID, AllowListFragment.FACTORY_PID);
+ this.allowList = allowList;
+ final String name = Converters.standardConverter().convert(config.get(LEGACY_NAME)).to(String.class);
+ final String[] bundles = Converters.standardConverter().convert(config.get(LEGACY_BUNDLES)).to(String[].class);
+ this.fragment = new AllowListFragment(name, bundles);
+ this.allowList.bindAllowListFragment(fragment);
Review Comment:
I think this code is fine, but if you want to change it, please update this branch
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] sonarcloud[bot] commented on pull request #11: Issues/sling 11741
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1736258160
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list) [82.7% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-base&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] bdelacretaz commented on a diff in pull request #11: Issues/sling 11741
Posted by "bdelacretaz (via GitHub)" <gi...@apache.org>.
bdelacretaz commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1329749920
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,236 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class is updating configuration and configuration property names to use
+ * more inclusive language.
+ * See https://issues.apache.org/jira/browse/SLING-11741
+ */
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
Review Comment:
IIUC this updater is specific to this configuration name change, aren't we going to have more similar cases of configuration renaming? And if yes, shouldn't that class rather be generic in its own module?
We can always make it more generic later, but I'm curious about the intention.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1726968834
@jsedding I'll have a look to see what we can do at runtime *without* creating/deleting configurations
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337171961
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -83,23 +75,22 @@ public class LoginAdminAllowList {
cardinality = ReferenceCardinality.MULTIPLE,
policy = ReferencePolicy.DYNAMIC,
policyOption = ReferencePolicyOption.GREEDY
- ) @SuppressWarnings("unused")
+ )
void bindAllowListFragment(AllowListFragment fragment) {
allowListFragments.add(fragment);
LOG.info("AllowListFragment added '{}'", fragment);
}
- @SuppressWarnings("unused")
void unbindAllowListFragment(AllowListFragment fragment) {
allowListFragments.remove(fragment);
LOG.info("AllowListFragment removed '{}'", fragment);
}
- @Activate @Modified @SuppressWarnings("unused")
- void configure(LoginAdminAllowListConfiguration configuration, Map<String, Object> properties) {
- this.config = new ConfigurationState(configuration);
- ensureBackwardsCompatibility(properties, PROP_WHITELIST_BUNDLES_DEFAULT);
- ensureBackwardsCompatibility(properties, PROP_WHITELIST_BUNDLES_ADDITIONAL);
+ @Activate @Modified
+ void configure(final LoginAdminAllowListConfiguration configuration, final Map<String, Object> properties) {
Review Comment:
We could but we need the properties for all the methods below, so it is a little bit easier this way
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337495390
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
Review Comment:
Fixed.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1338631696
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
+ }
+ this.bypassAllowList = bypass;
+ if (this.bypassAllowList) {
LOG.info("bypassAllowlist=true, allowlisted BSNs=<ALL>");
LOG.warn("All bundles are allowed to use loginAdministrative due to the 'allowlist.bypass' " +
Review Comment:
Same as above
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#issuecomment-1725470467
I think we have two options: we do a clever implementation like we have here which does the conversion behind the scenes and warns users to switch to the new configuration. Or we make a breaking change and provide some other means to users to change their configuration before it gets deployed into configuration admin service. As there are many different ways for the latter, we tried here the former.
The goal is to eventually get rid of the support for the old format (breaking change) with some time for our users to adjust.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337173815
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -47,34 +45,28 @@
* use the loginAdministrative method.
*/
@Component(service = LoginAdminAllowList.class,
- configurationPid = LoginAdminAllowList.PID,
- reference = {
- // ConfigurationUpdater is a required dependency to make sure that configurations are
- // updated before this component is activated
- @Reference(
- name = "ConfigurationUpdater",
- service = ConfigurationUpdater.class,
- cardinality = ReferenceCardinality.MANDATORY
- )
- }
-)
-@Designate(
- ocd = LoginAdminAllowListConfiguration.class
+ configurationPid = {LoginAdminAllowList.PID, LoginAdminAllowList.LEGACY_PID}
Review Comment:
As there is no overlap in properties I think it does not matter
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] jsedding commented on a diff in pull request #11: Issues/sling 11741
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1337496228
##########
src/main/java/org/apache/sling/jcr/base/internal/LegacyFragment.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.converter.Converters;
+
+import java.util.Map;
+
+/**
+ * Legacy fragment configuration. Use {@link AllowListFragment} instead.
+ */
+@Component(
+ configurationPid = LegacyFragment.LEGACY_FACTORY_PID,
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = LegacyFragment.class
+)
+public class LegacyFragment {
+
+ public static final String LEGACY_FACTORY_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+
+ private static final String LEGACY_NAME = "whitelist.name";
+ private static final String LEGACY_BUNDLES = "whitelist.bundles";
+
+ private final AllowListFragment fragment;
+
+ private final LoginAdminAllowList allowList;
+
+ @Activate
+ public LegacyFragment(final @Reference LoginAdminAllowList allowList, final Map<String, Object> config) {
+ LoginAdminAllowList.LOG.warn("Using deprecated factory configuration '{}'. " +
+ "Update your configuration to use configuration '{}' instead.",
+ LEGACY_FACTORY_PID, AllowListFragment.FACTORY_PID);
+ this.allowList = allowList;
+ final String name = Converters.standardConverter().convert(config.get(LEGACY_NAME)).to(String.class);
+ final String[] bundles = Converters.standardConverter().convert(config.get(LEGACY_BUNDLES)).to(String[].class);
+ this.fragment = new AllowListFragment(name, bundles);
+ this.allowList.bindAllowListFragment(fragment);
Review Comment:
See ce1e6a9 for what I mean.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] cziegeler commented on a diff in pull request #11: Issues/sling 11741
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1338631416
##########
src/main/java/org/apache/sling/jcr/base/internal/LoginAdminAllowList.java:
##########
@@ -133,24 +124,47 @@ public boolean allowLoginAdministrative(Bundle b) {
}
// encapsulate configuration state for atomic configuration updates
- private static class ConfigurationState {
+ static class ConfigurationState {
+
+ public final boolean bypassAllowList;
- private final boolean bypassAllowList;
+ public final Pattern allowListRegexp;
- private final Pattern allowListRegexp;
+ ConfigurationState(final LoginAdminAllowListConfiguration config, final Map<String, Object> properties) {
+ // first check for legacy properties
+ boolean bypass = config.allowlist_bypass();
+ final Object legacyBypassObject = properties.get(LEGACY_BYPASS_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BYPASS_PROPERTY, LEGACY_PID, PID, "allowlist.bypass");
+ bypass = Converters.standardConverter().convert(legacyBypassObject).defaultValue(false).to(Boolean.class);
+ }
+ String legacyRegexp = null;
+ final Object legacyBundlesObject = properties.get(LEGACY_BUNDLES_PROPERTY);
+ if (legacyBypassObject != null) {
+ LOG.warn("Using deprecated configuration property '{}' from configuration '{}'. " +
+ "Update your configuration to use configuration '{}' and property '{}' instead.",
+ LEGACY_BUNDLES_PROPERTY, LEGACY_PID, PID, "allowlist.bundles.regexp");
+ legacyRegexp = Converters.standardConverter().convert(legacyBundlesObject).to(String.class);
+ }
- private ConfigurationState(final LoginAdminAllowListConfiguration config) {
final String regexp = config.allowlist_bundles_regexp();
- if(regexp.trim().length() > 0) {
- allowListRegexp = Pattern.compile(regexp);
- LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}",
- allowListRegexp);
+ if (regexp.trim().length() > 0) {
+ if (legacyRegexp != null) {
+ LOG.warn("Both deprecated configuration property '{}' and configuration property '{}' are set. " +
+ "The deprecated property '{}' is ignored.",
+ LEGACY_BUNDLES_PROPERTY, "allowlist.bundles.regexp", LEGACY_BUNDLES_PROPERTY);
+ }
+ this.allowListRegexp = Pattern.compile(regexp);
} else {
- allowListRegexp = null;
+ this.allowListRegexp = legacyRegexp != null ? Pattern.compile(legacyRegexp) : null;
}
-
- bypassAllowList = config.allowlist_bypass();
- if(bypassAllowList) {
+ if (this.allowListRegexp != null) {
+ LOG.warn("A 'allowlist.bundles.regexp' is configured, this is NOT RECOMMENDED for production: {}", allowListRegexp);
Review Comment:
if we do this we should do it in a different issue
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-jcr-base] joerghoh commented on a diff in pull request #11: Issues/sling 11741
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #11:
URL: https://github.com/apache/sling-org-apache-sling-jcr-base/pull/11#discussion_r1216527225
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
Review Comment:
Can you please provide a short comment what this class is all about?
##########
src/main/java/org/apache/sling/jcr/base/internal/ConfigurationUpdater.java:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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.sling.jcr.base.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.List;
+import java.util.Map;
+
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ConfigurationEvent;
+import org.osgi.service.cm.ConfigurationListener;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Component(service = {ConfigurationListener.class, ConfigurationUpdater.class})
+public class ConfigurationUpdater implements ConfigurationListener {
+
+ static final String LOGIN_ADMIN_WHITELIST_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist";
+ static final String LOGIN_ADMIN_ALLOWLIST_PID = LoginAdminAllowList.PID;
+ private static final Map<String, String> LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE = new HashMap<>();
+ static {
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bypass", "allowlist.bypass");
+ LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE.put("whitelist.bundles.regexp", "allowlist.bundles.regexp");
+ }
+
+ private static final String WHITELIST_FRAGMENT_PID = "org.apache.sling.jcr.base.internal.LoginAdminWhitelist.fragment";
+ private static final String ALLOWLIST_FRAGMENT_PID = AllowListFragment.FACTORY_PID;
+ private static final Map<String, String> FRAGMENT_PROPS_TO_REPLACE = new HashMap<>();
+
+ static {
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.name", "allowlist.name");
+ FRAGMENT_PROPS_TO_REPLACE.put("whitelist.bundles", "allowlist.bundles");
+ }
+
+ protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ private final List<Updater> configurationUpdaterList = new ArrayList<>();
+
+ private final ConfigurationAdmin configurationAdmin;
+
+ @Activate
+ public ConfigurationUpdater(@Reference ConfigurationAdmin configurationAdmin) {
+ this.configurationAdmin = configurationAdmin;
+ configurationUpdaterList.add(new PidConfigurationUpdater(LOGIN_ADMIN_WHITELIST_PID, LOGIN_ADMIN_ALLOWLIST_PID, LOGIN_ADMIN_WHITELIST_PROPS_TO_REPLACE));
+ configurationUpdaterList.add(new FactoryPidConfigurationUpdater(WHITELIST_FRAGMENT_PID, ALLOWLIST_FRAGMENT_PID, FRAGMENT_PROPS_TO_REPLACE));
+
+ configurationUpdaterList.forEach(configurationUpdater -> configurationUpdater.updateProps());
+ }
+
+ @Override
+ public void configurationEvent(final ConfigurationEvent event) {
+ if ( event.getType() == ConfigurationEvent.CM_UPDATED ) {
+ configurationUpdaterList.forEach(configurationUpdater -> {
+ configurationUpdater.updateProps(event);
+ });
+ }
+ }
+
+ /**
+ * Encode the value for the ldap filter: \, *, (, and ) should be escaped.
+ */
+ private static String encode(final String value) {
+ return value.replace("\\", "\\\\")
+ .replace("*", "\\*")
+ .replace("(", "\\(")
+ .replace(")", "\\)");
+ }
+
+ protected abstract class Updater {
+
+ protected final String oldPid;
+ protected final String newPid;
+ protected final Map<String, String> propsToReplace;
+
+ public Updater(final String oldPid, final String newPid, final Map<String, String> propsToReplace) {
+ this.oldPid = oldPid;
+ this.newPid = newPid;
+ this.propsToReplace = propsToReplace;
+ }
+
+ protected abstract void updateProps(ConfigurationEvent event);
+
+ protected abstract void updateProps();
+
+ protected abstract Configuration createConfiguration(String oldPid) throws IOException;
+
+ /**
+ * Update a configuration
+ */
+ protected void updateProps(final Configuration sourceConfig, ConfigurationAdmin configurationAdmin) {
+ final Dictionary<String, Object> sourceProps = sourceConfig.getProperties();
+ final Dictionary<String, Object> targetProps = new Hashtable<>();
+ for(final String name : Collections.list(sourceProps.keys())) {
+ targetProps.put(this.propsToReplace.getOrDefault(name, name), sourceProps.get(name));
+ }
+ try {
+ final Configuration cfg = this.createConfiguration(sourceConfig.getPid());
+ if (cfg==null) return;
+ logger.info("Creating new configuration with PID {} for source PID: {}", cfg.getPid(), sourceConfig.getPid());
+ cfg.update(targetProps);
+ logger.info("Deleting source configuration wuth PID {} after it was migrated", sourceConfig.getPid());
Review Comment:
I see 2 log statements for each update being performed. Suggestions:
* Can we merge it into a single statement?
* We should point that the developer/admin should change the configuration accordingly, maybe with a link to documentation or a JIRA ticket?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org