This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch UNOMI-978-action-identity-ownership
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to
refs/heads/UNOMI-978-action-identity-ownership by this push:
new 34e6e5277 UNOMI-978: Allowlist property writes and refuse untrusted
identity claims
34e6e5277 is described below
commit 34e6e5277466da41ffad822d9f3198c18e37e2a0
Author: Serge Huber <[email protected]>
AuthorDate: Tue Sep 22 20:42:43 2026 +0200
UNOMI-978: Allowlist property writes and refuse untrusted identity claims
Two ways remained for a public-key caller to reach the identity state the
previous commit gates on:
- UpdatePropertiesAction matched systemProperties by literal prefix, but
commons-beanutils also accepts the mapped form systemProperties(key),
which has no dot and writes into the same map. Reserved bean fields
(itemId, itemType, tenantId, version, mergedWith) were reachable too,
and a rewritten itemId is then saved over the other profile's document.
- MergeProfilesOnPropertyAction refused the merge for untrusted callers
but still recorded the caller-chosen merge identifier. A later trusted
login for that identifier merged the planted profile in and aliased it
onto the master.
Replace the systemProperties check with one allowlist, isWritable: the
name must be a plain dotted path (no mapped/indexed syntax, no empty
segments, no control characters) and its first segment must be
'properties' for a public caller, or additionally systemProperties,
segments, scores or consents for a trusted caller. Identity fields are
never writable. Move the merge trust gate ahead of the identifier write so
an untrusted login claims nothing, and share the trust policy in
IdentityTrust.
Add a table-driven intrusion suite covering the classic injection
families (beanutils gadget, escapes and encodings, mapped/indexed syntax,
reserved fields, path shape, hostile merge values), doPost coverage for
the login sample against an HTTP stub, a BaseIT helper for switching the
test subject, and an IT for the identity-field rewrite. BasicIT's first
login is now a trusted call, since a public login no longer records a
merge identifier. Update the manual pages that still described the
pre-gate model, remove the duplicate hardening anchor, drop the dead
--version flag from setup.sh, and correct the sample's servlet comment on
what the server-side session id does and does not link.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
---
.../test/java/org/apache/unomi/itests/BaseIT.java | 17 ++
.../test/java/org/apache/unomi/itests/BasicIT.java | 7 +-
.../org/apache/unomi/itests/ContextServletIT.java | 4 +
.../org/apache/unomi/itests/ProfileMergeIT.java | 20 +-
.../unomi/itests/PropertiesUpdateActionIT.java | 69 ++++--
manual/src/main/asciidoc/builtin-event-types.adoc | 11 +-
.../asciidoc/migrations/migrate-3.0-to-3.1.adoc | 3 +-
manual/src/main/asciidoc/recipes.adoc | 6 +-
manual/src/main/asciidoc/samples/login-sample.adoc | 6 +-
.../plugins/baseplugin/actions/IdentityTrust.java | 44 ++++
.../actions/MergeProfilesOnPropertyAction.java | 38 ++-
.../baseplugin/actions/UpdatePropertiesAction.java | 68 +++---
.../actions/ActionIdentityIntrusionTest.java | 271 +++++++++++++++++++++
.../actions/MergeProfilesOnPropertyActionTest.java | 44 +++-
.../actions/UpdatePropertiesActionTest.java | 88 +++++++
samples/login-integration/setup.sh | 6 +-
.../apache/unomi/samples/login/LoginServlet.java | 10 +-
.../unomi/samples/login/LoginServletTest.java | 149 +++++++++++
18 files changed, 757 insertions(+), 104 deletions(-)
diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
index 1940d3c79..f699cf9c9 100644
--- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
@@ -877,6 +877,23 @@ public abstract class BaseIT extends KarafTestSupport {
return value;
}
+ /**
+ * Runs {@code action} with the request subject temporarily replaced by a
tenant subject for
+ * {@link #TEST_TENANT_ID}: a tenant-admin (private key) subject when
{@code systemAccess} is
+ * true, a public-key subject otherwise. The suite {@code @Before}
installs a tenant-admin
+ * subject, so tests that need to behave as a public caller must downgrade
it, and must restore
+ * it afterwards or every later test in the shared container runs
unprivileged.
+ */
+ protected void runAsTenantSubject(boolean systemAccess, Runnable action) {
+ javax.security.auth.Subject previous =
securityService.getCurrentSubject();
+ try {
+
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
systemAccess));
+ action.run();
+ } finally {
+ securityService.setCurrentSubject(previous);
+ }
+ }
+
protected void waitForProfileProperty(String profileId, String
propertyName, Object expected)
throws InterruptedException {
keepTrying("Profile " + profileId + " property " + propertyName + "
not updated",
diff --git a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
index ff4e12f06..682ff5a1b 100644
--- a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
@@ -199,12 +199,15 @@ public class BasicIT extends BaseIT {
// Create login event with VISITOR_1
ContextRequest contextRequestLoginVisitor1 =
getContextRequestWithLoginEvent(sourceSite, loginEventPropertiesVisitor1,
EMAIL_VISITOR_1, SESSION_ID_3);
+ // Login events come from the trusted server side: only a trusted
caller may record the
+ // merge identifier that the later identity switch below relies on.
HttpPost requestLoginVisitor1 = new
HttpPost(getFullUrl("/cxs/context.json"));
requestLoginVisitor1.addHeader("Cookie",
requestResponsePageView1.getCookieHeaderValue());
- requestLoginVisitor1.addHeader("X-Unomi-Api-Key", testPublicKeyValue);
+ requestLoginVisitor1.setHeader("Authorization", "Basic " +
Base64.getEncoder().encodeToString(
+ (TEST_TENANT_ID + ":" + testPrivateKeyValue).getBytes()));
requestLoginVisitor1.setEntity(new
StringEntity(getObjectMapper().writeValueAsString(contextRequestLoginVisitor1),
ContentType.create("application/json")));
- TestUtils.RequestResponse requestResponseLoginVisitor1 =
executeContextJSONRequest(requestLoginVisitor1, SESSION_ID_3);
+ TestUtils.RequestResponse requestResponseLoginVisitor1 =
executeContextJSONRequest(requestLoginVisitor1, SESSION_ID_3, -1, false);
Assert.assertEquals("Context profile id should be the same",
profileIdVisitor1,
requestResponseLoginVisitor1.getContextResponse().getProfileId());
checkVisitor1ResponseProperties(requestResponseLoginVisitor1.getContextResponse().getProfileProperties());
diff --git a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java
b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java
index 247460e26..609cdd5be 100644
--- a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java
@@ -580,6 +580,10 @@ public class ContextServletIT extends BaseIT {
assertEquals("Public login must not take over the other profile",
publicCallerId,
afterLogin.getContextResponse().getProfileId());
assertNotNull(profileService.load(otherId));
+ shouldBeTrueUntilEnd("Public login must not record the claimed
identifier on the caller profile",
+ () -> profileService.load(publicCallerId),
+ p -> p == null ||
p.getSystemProperties().get("mergeIdentifier") == null,
+ DEFAULT_TRYING_TIMEOUT, DEFAULT_SHOULDBETRUE_TRIES);
rulesService.removeRule("testLogin");
}
diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
index 68853c2b4..f6952e67f 100644
--- a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
@@ -197,17 +197,15 @@ public class ProfileMergeIT extends BaseIT {
Session session = new Session("untrustedMergeSession", publicCaller,
new Date(), null);
Event event = new Event(TEST_EVENT_TYPE, session, publicCaller, null,
null, publicCaller, new Date());
- javax.security.auth.Subject previous =
securityService.getCurrentSubject();
- try {
-
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
false));
- eventService.send(event);
- } finally {
- securityService.setCurrentSubject(previous);
- }
+ runAsTenantSubject(false, () -> eventService.send(event));
Assert.assertEquals("publicCallerProfileID",
event.getProfile().getItemId());
Assert.assertEquals("publicCallerProfileID",
event.getSession().getProfile().getItemId());
Assert.assertNotNull(profileService.load("otherProfileID"));
+ // The identity claim must not be recorded either: a planted
mergeIdentifier is what a later
+ // trusted login for [email protected] would merge on, pulling this
profile into that merge.
+ Assert.assertNull("untrusted caller must not record a merge
identifier",
+ publicCaller.getSystemProperties().get("mergeIdentifier"));
}
@Test
@@ -229,13 +227,7 @@ public class ProfileMergeIT extends BaseIT {
Session session = new Session("trustedMergeSession", caller, new
Date(), null);
Event event = new Event(TEST_EVENT_TYPE, session, caller, null, null,
caller, new Date());
- javax.security.auth.Subject previous =
securityService.getCurrentSubject();
- try {
-
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
true));
- eventService.send(event);
- } finally {
- securityService.setCurrentSubject(previous);
- }
+ runAsTenantSubject(true, () -> eventService.send(event));
Assert.assertEquals("trustedOtherProfileID",
event.getProfile().getItemId());
Assert.assertEquals("trustedOtherProfileID",
event.getSession().getProfile().getItemId());
diff --git
a/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
b/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
index 319fabc0f..405440bae 100644
--- a/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
@@ -133,14 +133,7 @@ public class PropertiesUpdateActionIT extends BaseIT {
updateProperties.setProperty(UpdatePropertiesAction.TARGET_ID_KEY,
PROFILE_TEST_ID);
updateProperties.setProperty(UpdatePropertiesAction.TARGET_TYPE_KEY,
"profile");
- javax.security.auth.Subject previous =
securityService.getCurrentSubject();
- try {
-
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
false));
- int changes = eventService.send(updateProperties);
- Assert.assertEquals(EventService.NO_CHANGE, changes);
- } finally {
- securityService.setCurrentSubject(previous);
- }
+ runAsTenantSubject(false, () ->
Assert.assertEquals(EventService.NO_CHANGE,
eventService.send(updateProperties)));
shouldBeTrueUntilEnd("Other profile must remain unchanged",
() -> profileService.load(PROFILE_TEST_ID),
@@ -159,17 +152,55 @@ public class PropertiesUpdateActionIT extends BaseIT {
propertyToUpdate.put("systemProperties.mergeIdentifier", "reused");
updateProperties.setProperty(UpdatePropertiesAction.PROPS_TO_UPDATE,
propertyToUpdate);
- javax.security.auth.Subject previous =
securityService.getCurrentSubject();
- try {
-
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
false));
- eventService.send(updateProperties);
- } finally {
- securityService.setCurrentSubject(previous);
- }
+ runAsTenantSubject(false, () -> eventService.send(updateProperties));
Assert.assertNull(profileService.load(PROFILE_TEST_ID).getSystemProperties().get("mergeIdentifier"));
}
+ /**
+ * Same write through the commons-beanutils mapped syntax, which has no
dot and therefore
+ * escaped a prefix match on {@code systemProperties.}.
+ */
+ @Test
+ public void
testUntrustedCaller_cannotWriteSystemPropertiesViaMappedSyntax() throws
InterruptedException {
+ Profile caller = profileService.load(PROFILE_TEST_ID);
+ Assert.assertNull(caller.getSystemProperties().get("mergeIdentifier"));
+
+ Event updateProperties = new Event("updateProperties", null, caller,
null, null, null, new Date());
+ updateProperties.setPersistent(false);
+ Map<String, Object> propertyToUpdate = new HashMap<>();
+ propertyToUpdate.put("systemProperties(mergeIdentifier)",
"[email protected]");
+ updateProperties.setProperty(UpdatePropertiesAction.PROPS_TO_UPDATE,
propertyToUpdate);
+
+ runAsTenantSubject(false, () ->
Assert.assertEquals(EventService.NO_CHANGE,
eventService.send(updateProperties)));
+
+
Assert.assertNull(profileService.load(PROFILE_TEST_ID).getSystemProperties().get("mergeIdentifier"));
+ }
+
+ /**
+ * Reserved bean fields are reachable through the same property path as
ordinary properties;
+ * rewriting {@code itemId} would make the next save overwrite another
profile's document.
+ */
+ @Test
+ public void testUntrustedCaller_cannotRewriteProfileIdentityFields()
throws InterruptedException {
+ Profile caller = profileService.load(PROFILE_TEST_ID);
+
+ Event updateProperties = new Event("updateProperties", null, caller,
null, null, null, new Date());
+ updateProperties.setPersistent(false);
+ Map<String, Object> propertyToUpdate = new HashMap<>();
+ propertyToUpdate.put("itemId", PROFILE_TARGET_TEST_ID);
+ propertyToUpdate.put("mergedWith", PROFILE_TARGET_TEST_ID);
+ propertyToUpdate.put("segments", new
java.util.ArrayList<>(java.util.Collections.singletonList("vip")));
+ updateProperties.setProperty(UpdatePropertiesAction.PROPS_TO_UPDATE,
propertyToUpdate);
+
+ runAsTenantSubject(false, () ->
Assert.assertEquals(EventService.NO_CHANGE,
eventService.send(updateProperties)));
+
+ Assert.assertEquals(PROFILE_TEST_ID, caller.getItemId());
+ Assert.assertNull(caller.getMergedWith());
+ Assert.assertTrue(caller.getSegments().isEmpty());
+
Assert.assertNull(profileService.load(PROFILE_TARGET_TEST_ID).getProperty("firstName"));
+ }
+
@Test
public void testTrustedPrivateKeySubject_canUpdateAnotherProfile() throws
InterruptedException {
Profile caller = profileService.load(PROFILE_TARGET_TEST_ID);
@@ -183,13 +214,7 @@ public class PropertiesUpdateActionIT extends BaseIT {
updateProperties.setProperty(UpdatePropertiesAction.TARGET_ID_KEY,
PROFILE_TEST_ID);
updateProperties.setProperty(UpdatePropertiesAction.TARGET_TYPE_KEY,
"profile");
- javax.security.auth.Subject previous =
securityService.getCurrentSubject();
- try {
-
securityService.setCurrentSubject(securityService.createSubject(TEST_TENANT_ID,
true));
- eventService.send(updateProperties);
- } finally {
- securityService.setCurrentSubject(previous);
- }
+ runAsTenantSubject(true, () -> eventService.send(updateProperties));
waitForProfileProperty(PROFILE_TEST_ID, "firstName", "TRUSTED UPDATE");
}
diff --git a/manual/src/main/asciidoc/builtin-event-types.adoc
b/manual/src/main/asciidoc/builtin-event-types.adoc
index 8a4402a31..0067a6055 100644
--- a/manual/src/main/asciidoc/builtin-event-types.adoc
+++ b/manual/src/main/asciidoc/builtin-event-types.adoc
@@ -245,9 +245,14 @@ This event is usually used by user interfaces that make it
possible to modify pr
Note that this event type is a protected event type that is only accepted from
configured third-party servers, which are
authorized by source IP address (and in V2 compatibility mode by an
`X-Unomi-Peer` key valid for that address). That
gate is separate from the public-versus-trusted caller distinction that
governs which profile and session a request may
-bind to — see <<_client_facing_hardening_3_1,client-facing hardening>>. Once
the event is accepted, its `targetId`
-cross-profile update and its `systemProperties.*` writes are not subject to a
further caller check, so the address
-allowlist is what protects them.
+bind to — see <<_client_facing_hardening_3_1,client-facing hardening>>. Once
the event is accepted, the property
+names in `add`, `update`, `addToSet` and `delete` are checked against an
allowlist. A public-key caller may only write
+plain dotted paths under `properties` (for example `properties.address.city`).
A trusted caller (system administrator
+or tenant private key) may also write `systemProperties`, `segments`, `scores`
and `consents`, and may target another
+profile through `targetId`. Identity and bookkeeping fields of the profile
(`itemId`, `itemType`, `tenantId`,
+`version`, `mergedWith`, ...) are never writable, and names using beanutils
mapped or indexed syntax (`a(b)`, `a[0]`),
+empty path segments or control characters are rejected for everyone. Refused
names are skipped with a `WARN` log entry;
+the rest of the event still applies.
===== Structure definition
diff --git a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc
b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc
index 59350e8ec..56edd5da1 100644
--- a/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc
+++ b/manual/src/main/asciidoc/migrations/migrate-3.0-to-3.1.adoc
@@ -256,7 +256,6 @@ When migrating to 3.1, you need to understand that:
- Public `/context.json` and `/eventcollector` no longer treat body/query
`profileId` as identity — see below
- Endpoint paths and most payload shapes remain the same; authentication
and public profile-binding rules change
-[#_client_facing_hardening_3_1]
==== Client-facing hardening (profile cookie, passwords, privileged APIs)
In addition to tenant API keys, Unomi 3.1 hardens several contracts that can
break older clients and scripts. See also <<_how_profile_tracking_works,How
profile tracking works>> and
https://issues.apache.org/jira/browse/UNOMI-972[UNOMI-972].
@@ -293,7 +292,7 @@ In addition to tenant API keys, Unomi 3.1 hardens several
contracts that can bre
|`mergeProfilesOnProperty` / cross-profile `updateProperties` /
`systemProperties.*`
|Sometimes driven from public context events
-|Cross-profile merge/update and `systemProperties` writes require a trusted
caller (system or tenant admin). Public callers may still update the
**current** cookie-bound profile's normal properties when the event type allows
it.
+|Cross-profile merge/update and `systemProperties` writes require a trusted
caller (system or tenant admin); this includes recording the merge identifier
on the current profile, so a public `login` event no longer claims an identity
at all. `updateProperties` now applies an allowlist to property names: public
callers may only write plain dotted `properties.*` paths on the **current**
cookie-bound profile; `systemProperties`, `segments`, `scores` and `consents`
need a trusted caller; iden [...]
|===
[WARNING]
diff --git a/manual/src/main/asciidoc/recipes.adoc
b/manual/src/main/asciidoc/recipes.adoc
index 84b74a1ab..f2a704791 100644
--- a/manual/src/main/asciidoc/recipes.adoc
+++ b/manual/src/main/asciidoc/recipes.adoc
@@ -137,8 +137,10 @@ defining the corresponding JSON schema and you're ready to
update profiles using
only. Being a protected event type, it is only accepted from an authorized
source address — a configured third-party
server, or in V2 compatibility mode an `X-Unomi-Peer` key valid for that
source address. Note that this is an
address-based gate: it is independent of the public-versus-trusted caller
distinction that governs profile and session
-binding (see <<_client_facing_hardening_3_1,client-facing hardening>>), and
once the event is accepted its `targetId`
-and `systemProperties.*` writes are not further restricted. Prefer custom
events for public visitors. Again, prefer the
+binding (see <<_client_facing_hardening_3_1,client-facing hardening>>), and
once the event is accepted a public-key
+caller may only write plain `properties.*` paths on the current profile;
`targetId`, `systemProperties`, `segments`,
+`scores` and `consents` require a trusted caller (system administrator or
tenant private key), and identity fields such
+as `itemId` are never writable. Refused names are skipped with a `WARN` log
entry. Prefer custom events for public visitors. Again, prefer the
custom events solution because as this is a
protected event it will require sending trusted credentials, and as Unomi only
supports a single key for the moment it
could be problematic if the key is intercepted. But at least by using an event
you will get the benefits of auditing and
diff --git a/manual/src/main/asciidoc/samples/login-sample.adoc
b/manual/src/main/asciidoc/samples/login-sample.adoc
index 8e2ae12fb..93e93015c 100644
--- a/manual/src/main/asciidoc/samples/login-sample.adoc
+++ b/manual/src/main/asciidoc/samples/login-sample.adoc
@@ -95,7 +95,11 @@ trusted credentials hands that power straight back to the
browser: anyone who gu
visitor's session id could rebind or merge that visitor's profile.
`LoginServlet` therefore derives the session id from state it controls — a
UUID stored on the
-browser's container `HttpSession` — and never reads it from a request
parameter. Apply the same rule
+browser's container `HttpSession` — and never reads it from a request
parameter. The consequence is
+that the anonymous profile the browser built up through the public tracker
before logging in is
+*not* linked by this sample: doing so requires forwarding a browser-chosen
identifier (the profile
+cookie or the tracker's session id) under trusted credentials, which your
server must only do once
+it has established that the identifier belongs to the caller. Apply the same
rule
in your own integration:
* Derive the `sessionId` from your authenticated server-side session, not from
the request body.
diff --git
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IdentityTrust.java
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IdentityTrust.java
new file mode 100644
index 000000000..c4052042e
--- /dev/null
+++
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IdentityTrust.java
@@ -0,0 +1,44 @@
+/*
+ * 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.unomi.plugins.baseplugin.actions;
+
+import org.apache.unomi.api.security.SecurityService;
+import org.apache.unomi.api.security.UnomiRoles;
+
+/**
+ * Single definition of the trust policy for identity claims made through
actions: merging
+ * profiles, recording a merge identifier, updating another profile, or writing
+ * {@code systemProperties}.
+ * <p>
+ * This is a role check, not a check of the credential that produced it: a
tenant private key
+ * authenticates as {@link UnomiRoles#TENANT_ADMINISTRATOR} and therefore
passes, while a tenant
+ * public API key or an unauthenticated context event does not. A missing
+ * {@link SecurityService} fails closed.
+ */
+final class IdentityTrust {
+
+ private IdentityTrust() {
+ }
+
+ /**
+ * @param securityService the security service, possibly null while OSGi
wiring is in flux
+ * @return whether the caller holds system access (administrator or tenant
administrator)
+ */
+ static boolean isTrustedIdentityCaller(SecurityService securityService) {
+ return securityService != null && securityService.hasSystemAccess();
+ }
+}
diff --git
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java
index ff4832cc3..7ef891b24 100644
---
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java
+++
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java
@@ -26,7 +26,6 @@ import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.conditions.Condition;
import org.apache.unomi.api.security.SecurityService;
-import org.apache.unomi.api.security.UnomiRoles;
import org.apache.unomi.api.services.*;
import org.apache.unomi.persistence.spi.PersistenceService;
import org.slf4j.Logger;
@@ -112,6 +111,20 @@ public class MergeProfilesOnPropertyAction implements
ActionExecutor {
return EventService.PROFILE_UPDATED +
EventService.SESSION_UPDATED;
}
+ // Recording the merge identifier is itself an identity claim, not
just the merge that
+ // may follow it: a value persisted here is what a later, trusted
login for the same
+ // identifier queries on, so an untrusted caller that could plant
it would have its
+ // profile pulled into that merge (and aliased onto the master)
once the real owner logs
+ // in. Refuse the whole step, not only the merge, for callers
without system access.
+ if (!isTrustedIdentityCaller()) {
+ LOGGER.warn("Refusing merge identity claim for untrusted
caller (mergeProp={}, candidates={})",
+ mergePropName, profilesToBeMerge.size());
+ if (tracer != null) {
+ tracer.endOperation(false, "Untrusted caller cannot claim
a merge identity");
+ }
+ return EventService.NO_CHANGE;
+ }
+
// Store merge prop on current profile
boolean profileUpdated = false;
if (StringUtils.isEmpty(currentProfileMergeValue)) {
@@ -127,18 +140,6 @@ public class MergeProfilesOnPropertyAction implements
ActionExecutor {
return profileUpdated ? EventService.PROFILE_UPDATED :
EventService.NO_CHANGE;
}
- // Merging into another existing profile rebinds the session —
require a caller holding
- // system access (see isTrustedIdentityCaller), not a
public/unauthenticated event.
- if (!isTrustedIdentityCaller()) {
- LOGGER.warn("Refusing profile merge for untrusted caller
(mergeProp={}, candidates={})",
- mergePropName, profilesToBeMerge.size());
- if (tracer != null) {
- tracer.endOperation(false, "Untrusted caller cannot merge
into another profile");
- }
- // Keep only the merge identifier write on the current profile
when it was empty
- return profileUpdated ? EventService.PROFILE_UPDATED :
EventService.NO_CHANGE;
- }
-
// add current Profile to profiles to be merged
if (profilesToBeMerge.stream().noneMatch(p ->
StringUtils.equals(p.getItemId(), eventProfile.getItemId()))) {
profilesToBeMerge.add(eventProfile);
@@ -348,15 +349,12 @@ public class MergeProfilesOnPropertyAction implements
ActionExecutor {
}
/**
- * Whether the caller holds system access, i.e. the administrator or
tenant administrator role.
- * <p>
- * This is a role check, not a check of the credential that produced it: a
tenant private key
- * authenticates as {@link UnomiRoles#TENANT_ADMINISTRATOR} and therefore
passes, while a tenant
- * public API key or an unauthenticated context event does not. Identity
merges rebind sessions,
- * so they are restricted to callers that hold that role.
+ * Identity claims (switching, recording or merging on a merge identifier)
are restricted to
+ * callers holding system access; see {@link IdentityTrust}. This applies
to a switch even when
+ * no candidate profile exists yet: the switch still rebinds the session
to a new profile.
*/
private boolean isTrustedIdentityCaller() {
- return securityService != null && securityService.hasSystemAccess();
+ return IdentityTrust.isTrustedIdentityCaller(securityService);
}
public void setProfileService(ProfileService profileService) {
diff --git
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
index 78070d144..005a258c7 100644
---
a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
+++
b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java
@@ -26,7 +26,6 @@ import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.security.SecurityService;
import org.apache.unomi.api.utils.LogSanitizer;
-import org.apache.unomi.api.security.UnomiRoles;
import org.apache.unomi.api.services.EventService;
import org.apache.unomi.api.services.ProfileService;
import org.apache.unomi.persistence.spi.PropertyHelper;
@@ -36,6 +35,7 @@ import org.apache.unomi.tracing.api.TracerService;
import org.apache.unomi.tracing.api.RequestTracer;
import java.util.*;
+import java.util.regex.Pattern;
public class UpdatePropertiesAction implements ActionExecutor {
@@ -49,10 +49,21 @@ public class UpdatePropertiesAction implements
ActionExecutor {
public static final String TARGET_TYPE_PROFILE = "profile";
- /** The reserved profile field that only a caller with system access may
write. */
- private static final String SYSTEM_PROPERTIES_KEY = "systemProperties";
- /** Prefix of the profile properties that only a caller with system access
may write. */
- private static final String SYSTEM_PROPERTIES_PREFIX =
SYSTEM_PROPERTIES_KEY + ".";
+ /**
+ * The only profile areas an event may write, by caller trust. Everything
else on the bean
+ * ({@code itemId}, {@code itemType}, {@code tenantId}, {@code version},
{@code mergedWith},
+ * {@code scope}, {@code systemMetadata}, ...) is identity or bookkeeping
and is never writable
+ * through an event: an allowlist needs no knowledge of what a future
field might be called.
+ */
+ private static final Set<String> PUBLIC_WRITABLE_AREAS =
Set.of("properties");
+ private static final Set<String> TRUSTED_WRITABLE_AREAS =
Set.of("properties", "systemProperties", "segments", "scores", "consents");
+ /**
+ * A plain dotted path: non-empty segments, no control characters, and
none of the
+ * commons-beanutils mapped/indexed syntax ({@code a(b)}, {@code a[0]})
that
+ * {@link PropertyHelper#setProperty} would otherwise interpret.
Everything this action
+ * legitimately writes is expressible as {@code area.key.subkey}.
+ */
+ private static final Pattern PROPERTY_PATH =
Pattern.compile("[^.()\\[\\]\\p{Cntrl}]+(?:\\.[^.()\\[\\]\\p{Cntrl}]+)*");
private static final Logger LOGGER =
LoggerFactory.getLogger(UpdatePropertiesAction.class.getName());
@@ -124,8 +135,9 @@ public class UpdatePropertiesAction implements
ActionExecutor {
List<String> propsToDelete = (List<String>)
event.getProperties().get(PROPS_TO_DELETE);
if (propsToDelete != null) {
for (String prop : propsToDelete) {
- if (!trustedCaller && isSystemPropertiesWrite(prop)) {
- LOGGER.warn("Refusing systemProperties delete for
untrusted caller: {}", LogSanitizer.forLogging(prop));
+ if (!isWritable(prop, trustedCaller)) {
+ LOGGER.warn("Refusing property delete for {} caller:
{}", trustedCaller ? "trusted" : "untrusted",
+ LogSanitizer.forLogging(prop));
continue;
}
isProfileOrPersonaUpdated |=
PropertyHelper.setProperty(target, prop, null, "remove");
@@ -166,12 +178,13 @@ public class UpdatePropertiesAction implements
ActionExecutor {
private boolean processProperties(Profile target, Map<String, Object>
propsMap, String strategy, boolean trustedCaller) {
boolean isProfileOrPersonaUpdated = false;
for (String prop : propsMap.keySet()) {
- if (!trustedCaller && isSystemPropertiesWrite(prop)) {
- LOGGER.warn("Refusing systemProperties update for untrusted
caller: {}", LogSanitizer.forLogging(prop));
+ if (!isWritable(prop, trustedCaller)) {
+ LOGGER.warn("Refusing property write for {} caller: {}",
trustedCaller ? "trusted" : "untrusted",
+ LogSanitizer.forLogging(prop));
continue;
}
PropertyType propType = null;
- if (prop.startsWith("properties.") ||
prop.startsWith(SYSTEM_PROPERTIES_PREFIX)) {
+ if (prop.startsWith("properties.") ||
prop.startsWith("systemProperties.")) {
propType =
profileService.getPropertyType(prop.substring(prop.indexOf('.') + 1));
} else {
propType = profileService.getPropertyType(prop);
@@ -191,30 +204,29 @@ public class UpdatePropertiesAction implements
ActionExecutor {
/**
- * Whether a property name writes the reserved {@code systemProperties}
area.
+ * Whether an event-supplied property name may be written by this caller.
* <p>
- * Matching the {@code systemProperties.} prefix alone was not enough: the
bare key
- * {@code systemProperties} has no dot, so it slipped through, and for a
flat name
- * {@link org.apache.unomi.persistence.spi.PropertyHelper#setProperty}
falls through to
- * {@code BeanUtils.setProperty}, which invokes {@code
Profile#setSystemProperties(Map)} and
- * replaces the entire map. That is strictly more than the per-key write
this gate blocks — it
- * is enough to plant {@code mergeIdentifier} and drive the profile-merge
action.
+ * One rule covers every injection shape seen against this action: the
name must be a plain
+ * dotted path (so beanutils mapped/indexed syntax, empty segments and
control characters are
+ * out), and its first segment must be one of the areas allowed for the
caller's trust level
+ * (so identity fields such as {@code itemId} or {@code mergedWith}, and
for public callers
+ * {@code systemProperties}, {@code segments}, {@code scores} and {@code
consents}, are out).
*
* @param propertyName the event-supplied property name
- * @return true when the name targets systemProperties, whether wholesale
or a single entry
+ * @param trustedCaller whether the caller holds system access
+ * @return true when the write is allowed
*/
- private static boolean isSystemPropertiesWrite(String propertyName) {
- return SYSTEM_PROPERTIES_KEY.equals(propertyName) ||
propertyName.startsWith(SYSTEM_PROPERTIES_PREFIX);
+ static boolean isWritable(String propertyName, boolean trustedCaller) {
+ if (propertyName == null ||
!PROPERTY_PATH.matcher(propertyName).matches()) {
+ return false;
+ }
+ int dot = propertyName.indexOf('.');
+ String area = dot < 0 ? propertyName : propertyName.substring(0, dot);
+ return (trustedCaller ? TRUSTED_WRITABLE_AREAS :
PUBLIC_WRITABLE_AREAS).contains(area);
}
- /**
- * Whether the caller holds system access, i.e. the administrator or
tenant administrator role.
- * <p>
- * Cross-profile updates and {@code systemProperties.*} writes are
restricted to such callers.
- * A tenant private key authenticates as {@link
UnomiRoles#TENANT_ADMINISTRATOR} and passes; a
- * tenant public API key or an unauthenticated context event does not.
- */
+
private boolean isTrustedIdentityCaller() {
- return securityService != null && securityService.hasSystemAccess();
+ return IdentityTrust.isTrustedIdentityCaller(securityService);
}
public void setProfileService(ProfileService profileService) {
diff --git
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/ActionIdentityIntrusionTest.java
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/ActionIdentityIntrusionTest.java
new file mode 100644
index 000000000..ac03cb3ff
--- /dev/null
+++
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/ActionIdentityIntrusionTest.java
@@ -0,0 +1,271 @@
+/*
+ * 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.unomi.plugins.baseplugin.actions;
+
+import org.apache.unomi.api.Event;
+import org.apache.unomi.api.PartialList;
+import org.apache.unomi.api.Profile;
+import org.apache.unomi.api.actions.Action;
+import org.apache.unomi.api.conditions.ConditionType;
+import org.apache.unomi.api.security.SecurityService;
+import org.apache.unomi.api.services.DefinitionsService;
+import org.apache.unomi.api.services.EventService;
+import org.apache.unomi.api.services.PrivacyService;
+import org.apache.unomi.api.services.ProfileService;
+import org.apache.unomi.persistence.spi.PersistenceService;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Intrusion scenarios against the two identity-bearing actions, run the way a
public-key caller
+ * would drive them: every classic injection shape for a property path, every
reserved field of the
+ * profile bean, and hostile merge values. Each scenario asserts on the
resulting profile state, not
+ * on the return code alone, so a regression in either action (or in
PropertyHelper, which resolves
+ * the names through commons-beanutils) shows up as a concrete unwanted write.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class ActionIdentityIntrusionTest {
+
+ @Mock private ProfileService profileService;
+ @Mock private PersistenceService persistenceService;
+ @Mock private EventService eventService;
+ @Mock private DefinitionsService definitionsService;
+ @Mock private PrivacyService privacyService;
+ @Mock private SecurityService securityService;
+
+ private UpdatePropertiesAction updateAction;
+ private MergeProfilesOnPropertyAction mergeAction;
+
+ /** Property paths a public caller must never be able to write through,
whatever the value. */
+ private static final List<String> HOSTILE_PATHS = Arrays.asList(
+ // identity and bookkeeping fields of the bean
+ "itemId", "itemType", "tenantId", "version", "mergedWith",
"scope", "systemMetadata",
+ // trust-bearing areas reserved to trusted callers
+ "systemProperties", "systemProperties.mergeIdentifier",
"systemProperties.mergedWith",
+ "segments", "scores.vip", "consents.newsletter",
+ // beanutils mapped / indexed / chained syntax
+ "systemProperties(mergeIdentifier)", "systemProperties[0]",
"systemProperties(a).b",
+ "systemProperties(a)(b)", "properties(a.b)", "properties[0]",
+ // path shape tricks: empty segments, leading/trailing dots,
control characters
+ "properties..x", "properties.",
".systemProperties.mergeIdentifier",
+ "properties.a\tb", "properties.a\nb", "properties\u0000.x",
+ // the classic commons-beanutils gadget
+ "class.classLoader.URLs", "class",
+ // encodings and lookalikes that never resolve, listed so a future
decoder cannot
+ // quietly turn them into the real thing
+ "systemProperties%2EmergeIdentifier",
"systemProperties\\.mergeIdentifier",
+ "SystemProperties.mergeIdentifier",
"\uFF53ystemProperties.mergeIdentifier",
+ "systemProperties\u200B.mergeIdentifier");
+
+ /** Values covering the shapes JSON can deliver: scalars, collections, and
nested maps. */
+ private static final List<Object> HOSTILE_VALUES = Arrays.asList(
+ "victim-profile-id",
+ Collections.singletonMap("mergeIdentifier", "[email protected]"),
+ new ArrayList<>(Arrays.asList("vip", "admin")),
+ Long.valueOf(99),
+ Boolean.TRUE);
+
+ @Before
+ public void setUp() {
+ updateAction = new UpdatePropertiesAction();
+ updateAction.setProfileService(profileService);
+ updateAction.setEventService(eventService);
+ updateAction.setSecurityService(securityService);
+
+ mergeAction = new MergeProfilesOnPropertyAction();
+ mergeAction.setProfileService(profileService);
+ mergeAction.setPersistenceService(persistenceService);
+ mergeAction.setEventService(eventService);
+ mergeAction.setDefinitionsService(definitionsService);
+ mergeAction.setPrivacyService(privacyService);
+ mergeAction.bindSecurityService(securityService);
+ mergeAction.setMaxProfilesInOneMerge("50");
+
+ when(securityService.hasSystemAccess()).thenReturn(false);
+ }
+
+ @Test
+ public void
publicCaller_hostilePathsLeaveEveryReservedFieldUntouched_add_update_addToSet()
{
+ for (String strategyKey : new String[] {
+ UpdatePropertiesAction.PROPS_TO_ADD,
UpdatePropertiesAction.PROPS_TO_UPDATE,
UpdatePropertiesAction.PROPS_TO_ADD_TO_SET}) {
+ for (String path : HOSTILE_PATHS) {
+ for (Object value : HOSTILE_VALUES) {
+ Profile caller = freshCaller();
+ Map<String, Object> props = new HashMap<>();
+ props.put(path, value);
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(strategyKey, props);
+
+ int changes = updateAction.execute(new Action(),
event(caller, eventProps));
+
+ String scenario = strategyKey + " " + printable(path) + "
= " + value;
+ assertEquals(scenario, EventService.NO_CHANGE, changes);
+ assertUntouched(scenario, caller);
+ }
+ }
+ }
+ }
+
+ @Test
+ public void publicCaller_hostilePathsCannotDeleteReservedFields() {
+ for (String path : HOSTILE_PATHS) {
+ Profile caller = freshCaller();
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_DELETE,
Collections.singletonList(path));
+
+ int changes = updateAction.execute(new Action(), event(caller,
eventProps));
+
+ assertEquals("delete " + printable(path), EventService.NO_CHANGE,
changes);
+ assertUntouched("delete " + printable(path), caller);
+ }
+ }
+
+ @Test
+ public void publicCaller_mixedBatchAppliesOnlyTheOrdinaryProperty() {
+ Profile caller = freshCaller();
+ Map<String, Object> props = new LinkedHashMap<>();
+ props.put("properties.firstName", "Alice");
+ props.put("systemProperties.mergeIdentifier", "[email protected]");
+ props.put("itemId", "victim-profile-id");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, props);
+
+ int changes = updateAction.execute(new Action(), event(caller,
eventProps));
+
+ assertEquals(EventService.PROFILE_UPDATED, changes);
+ assertEquals("Alice", caller.getProperty("firstName"));
+ assertUntouched("mixed batch", caller);
+ }
+
+ @Test
+ public void
publicCaller_cannotReachAnotherProfileThroughTargetIdVariants() {
+ for (String targetId : Arrays.asList("victim-profile-id", "
victim-profile-id", "victim-profile-id\n", "../victim")) {
+ for (String targetType : Arrays.asList("profile", "persona", "",
"PROFILE", null)) {
+ Profile caller = freshCaller();
+ Map<String, Object> props = new HashMap<>();
+ props.put("properties.firstName", "Mallory");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.TARGET_ID_KEY, targetId);
+ eventProps.put(UpdatePropertiesAction.TARGET_TYPE_KEY,
targetType);
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, props);
+
+ int changes = updateAction.execute(new Action(), event(caller,
eventProps));
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertNull("caller's own profile must not be written either",
caller.getProperty("firstName"));
+ }
+ }
+ verify(profileService, never()).load(anyString());
+ verify(profileService, never()).loadPersona(anyString());
+ verify(profileService, never()).save(any(Profile.class));
+ }
+
+ @Test
+ public void trustedCaller_reservedIdentityFieldsAreNeverWritable() {
+ when(securityService.hasSystemAccess()).thenReturn(true);
+ for (String path : Arrays.asList("itemId", "itemType", "tenantId",
"version", "mergedWith", "scope", "class.classLoader.URLs",
+ "systemProperties(mergeIdentifier)", "properties(a.b)",
"properties..x")) {
+ Profile caller = freshCaller();
+ Map<String, Object> props = new HashMap<>();
+ props.put(path, "victim-profile-id");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, props);
+
+ int changes = updateAction.execute(new Action(), event(caller,
eventProps));
+
+ assertEquals("trusted " + printable(path), EventService.NO_CHANGE,
changes);
+ assertUntouched("trusted " + printable(path), caller);
+ }
+ }
+
+ @Test
+ public void publicCaller_hostileMergeValuesNeverRecordOrMergeAnIdentity() {
+
when(definitionsService.getConditionType("profilePropertyCondition")).thenReturn(new
ConditionType());
+ Profile other = new Profile("victim-profile-id");
+ other.getSystemProperties().put("mergeIdentifier",
"[email protected]");
+ when(persistenceService.query(any(), anyString(), eq(Profile.class),
anyInt(), anyInt()))
+ .thenReturn(new PartialList<>(new
ArrayList<>(Collections.singletonList(other)), 0, 1, 1,
PartialList.Relation.EQUAL));
+
+ for (String value : Arrays.asList("[email protected]", "*",
"[email protected] OR 1=1", "[email protected]\n",
+ "{\"query\":{\"match_all\":{}}}", "victim-profile-id")) {
+ Profile caller = freshCaller();
+ Action action = new Action();
+ Map<String, Object> params = new HashMap<>();
+ params.put("mergeProfilePropertyName", "mergeIdentifier");
+ params.put("mergeProfilePropertyValue", value);
+ action.setParameterValues(params);
+ Event event = new Event("login", null, caller, "systemscope",
null, null, null, new Date(), true);
+
+ int changes = mergeAction.execute(action, event);
+
+ assertEquals(printable(value), EventService.NO_CHANGE, changes);
+ assertEquals(printable(value), "attacker",
event.getProfile().getItemId());
+ assertUntouched(printable(value), caller);
+ }
+ verify(profileService, never()).mergeProfiles(any(), any());
+ }
+
+ private static Profile freshCaller() {
+ Profile caller = new Profile("attacker");
+ caller.getSystemProperties().put("existing", "keep");
+ return caller;
+ }
+
+ private static Event event(Profile caller, Map<String, Object> eventProps)
{
+ return new Event("updateProperties", null, caller, "systemscope",
null, null, eventProps, new Date(), true);
+ }
+
+ /** Everything a public caller must not be able to change on its own
profile. */
+ private static void assertUntouched(String scenario, Profile caller) {
+ assertEquals(scenario, "attacker", caller.getItemId());
+ assertEquals(scenario, Profile.ITEM_TYPE, caller.getItemType());
+ assertNull(scenario, caller.getMergedWith());
+ assertNull(scenario, caller.getTenantId());
+ assertEquals(scenario, Collections.singletonMap("existing", "keep"),
caller.getSystemProperties());
+ assertTrue(scenario, caller.getSegments().isEmpty());
+ assertTrue(scenario, caller.getScores() == null ||
caller.getScores().isEmpty());
+ assertTrue(scenario, caller.getConsents() == null ||
caller.getConsents().isEmpty());
+ assertNull(scenario + " (map key 'class' would mean the gadget path
was reached)", caller.getProperty("class"));
+ }
+
+ private static String printable(String s) {
+ return s.replace("\t", "\\t").replace("\n", "\\n").replace("\u0000",
"\\0");
+ }
+}
diff --git
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyActionTest.java
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyActionTest.java
index 0ae1783aa..95a51a680 100644
---
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyActionTest.java
+++
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyActionTest.java
@@ -41,7 +41,7 @@ import java.util.HashMap;
import java.util.Map;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyString;
@@ -94,12 +94,50 @@ public class MergeProfilesOnPropertyActionTest {
int changes = actionExecutor.execute(action, event);
- // May write mergeIdentifier onto the untrusted caller profile, but
must not rebind to other
- assertNotEquals(EventService.PROFILE_UPDATED +
EventService.SESSION_UPDATED, changes);
+ assertEquals(EventService.NO_CHANGE, changes);
assertEquals("public-caller", event.getProfile().getItemId());
+ assertNull("the untrusted caller must not record the identity claim
either",
+ publicCaller.getSystemProperties().get("mergeIdentifier"));
verify(profileService, never()).mergeProfiles(any(), any());
}
+ /**
+ * The identifier write is the poisoning primitive, independently of
whether a candidate exists
+ * today: a value planted here is what a later trusted login for the same
identifier merges on,
+ * which would pull the planter's profile into the real owner's merge and
alias it onto the
+ * master. So the empty-candidates path must refuse too, not only the
merge path.
+ */
+ @Test
+ public void
untrustedCaller_cannotRecordMergeIdentifierEvenWithoutCandidates() {
+ Profile publicCaller = new Profile("public-caller");
+
+ when(persistenceService.query(any(), anyString(), eq(Profile.class),
anyInt(), anyInt()))
+ .thenReturn(new PartialList<>(new ArrayList<>(), 0, 0, 0,
PartialList.Relation.EQUAL));
+
+ Event event = new Event("login", null, publicCaller, "systemscope",
null, null, null, new Date(), true);
+
+ int changes =
actionExecutor.execute(mergeAction("[email protected]"), event);
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertNull(publicCaller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ @Test
+ public void
trustedTenantAdmin_recordsMergeIdentifierWhenNoCandidateExists() {
+ when(securityService.hasSystemAccess()).thenReturn(true);
+ Profile caller = new Profile("caller");
+
+ when(persistenceService.query(any(), anyString(), eq(Profile.class),
anyInt(), anyInt()))
+ .thenReturn(new PartialList<>(new ArrayList<>(), 0, 0, 0,
PartialList.Relation.EQUAL));
+
+ Event event = new Event("login", null, caller, "systemscope", null,
null, null, new Date(), true);
+
+ int changes = actionExecutor.execute(mergeAction("[email protected]"),
event);
+
+ assertEquals(EventService.PROFILE_UPDATED, changes);
+ assertEquals("[email protected]",
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
@Test
public void trustedTenantAdmin_canMergeIntoExistingProfile() {
when(securityService.hasSystemAccess()).thenReturn(true);
diff --git
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
index f1e33efd2..b0e9a3fe7 100644
---
a/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
+++
b/plugins/baseplugin/src/test/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesActionTest.java
@@ -190,4 +190,92 @@ public class UpdatePropertiesActionTest {
verify(profileService).load("other");
verify(profileService).save(other);
}
+
+ /**
+ * commons-beanutils also accepts the mapped syntax {@code
systemProperties(key)}. It has no
+ * dot, so PropertyHelper hands it to {@code BeanUtils.setProperty}
unchanged, and beanutils
+ * resolves it as {@code getSystemProperties().put(key, value)}: the same
write as the dotted
+ * form, which a prefix check on {@code systemProperties.} never saw.
+ */
+ @Test
+ public void untrustedCaller_cannotWriteSystemPropertiesViaMappedSyntax() {
+ Profile caller = new Profile("caller");
+ Map<String, Object> updateMap = new HashMap<>();
+ updateMap.put("systemProperties(mergeIdentifier)",
"[email protected]");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_UPDATE, updateMap);
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ int changes = actionExecutor.execute(new Action(), event);
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertEquals(null,
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ @Test
+ public void untrustedCaller_cannotAddToSystemPropertiesSet() {
+ Profile caller = new Profile("caller");
+ Map<String, Object> addToSetMap = new HashMap<>();
+ addToSetMap.put("systemProperties.mergeIdentifier",
"[email protected]");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_ADD_TO_SET,
addToSetMap);
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ int changes = actionExecutor.execute(new Action(), event);
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertEquals(null,
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ @Test
+ public void untrustedCaller_cannotDeleteASystemPropertiesEntry() {
+ Profile caller = new Profile("caller");
+ caller.getSystemProperties().put("mergeIdentifier",
"[email protected]");
+ Map<String, Object> eventProps = new HashMap<>();
+ eventProps.put(UpdatePropertiesAction.PROPS_TO_DELETE,
+
java.util.Collections.singletonList("systemProperties.mergeIdentifier"));
+
+ Event event = new Event("updateProperties", null, caller,
"systemscope", null, null, eventProps, new Date(), true);
+
+ int changes = actionExecutor.execute(new Action(), event);
+
+ assertEquals(EventService.NO_CHANGE, changes);
+ assertEquals("[email protected]",
caller.getSystemProperties().get("mergeIdentifier"));
+ }
+
+ @Test
+ public void writableCheck_isAnAllowlistOnAreaAndPathShape() {
+ // public callers: only plain dotted paths under properties
+ assertEquals(true,
UpdatePropertiesAction.isWritable("properties.firstName", false));
+ assertEquals(true,
UpdatePropertiesAction.isWritable("properties.address.city", false));
+ assertEquals(true,
UpdatePropertiesAction.isWritable("properties.j:nodename", false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("systemProperties.mergeIdentifier", false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("systemProperties(mergeIdentifier)", false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("systemProperties", false));
+ assertEquals(false, UpdatePropertiesAction.isWritable("segments",
false));
+ assertEquals(false, UpdatePropertiesAction.isWritable("scores.vip",
false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("consents.newsletter", false));
+ // identity and bookkeeping fields: nobody
+ assertEquals(false, UpdatePropertiesAction.isWritable("itemId", true));
+ assertEquals(false, UpdatePropertiesAction.isWritable("mergedWith",
true));
+ assertEquals(false, UpdatePropertiesAction.isWritable("tenantId",
true));
+ assertEquals(false, UpdatePropertiesAction.isWritable("version",
true));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("class.classLoader", true));
+ // trusted callers: the reserved areas, still as plain paths only
+ assertEquals(true,
UpdatePropertiesAction.isWritable("systemProperties.mergeIdentifier", true));
+ assertEquals(true,
UpdatePropertiesAction.isWritable("systemProperties", true));
+ assertEquals(true, UpdatePropertiesAction.isWritable("segments",
true));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("systemProperties(mergeIdentifier)", true));
+ assertEquals(false, UpdatePropertiesAction.isWritable("properties[0]",
true));
+ // path shape
+ assertEquals(false, UpdatePropertiesAction.isWritable("properties..x",
false));
+ assertEquals(false, UpdatePropertiesAction.isWritable("properties.",
false));
+ assertEquals(false, UpdatePropertiesAction.isWritable(".properties.x",
false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("properties.a\tb", false));
+ assertEquals(false,
UpdatePropertiesAction.isWritable("properties.a\nb", false));
+ assertEquals(false, UpdatePropertiesAction.isWritable("", false));
+ assertEquals(false, UpdatePropertiesAction.isWritable(null, false));
+ }
}
diff --git a/samples/login-integration/setup.sh
b/samples/login-integration/setup.sh
index ea56b13eb..37b15f89d 100755
--- a/samples/login-integration/setup.sh
+++ b/samples/login-integration/setup.sh
@@ -25,7 +25,7 @@
#
# Usage:
# export UNOMI_ROOT_PASSWORD='your-admin-password'
-# ./setup.sh [--version <sample-version>]
+# ./setup.sh
#
# Optional environment overrides:
# UNOMI_URL base URL of the running Unomi (default
http://localhost:8181)
@@ -41,14 +41,13 @@ TENANT_ID="${UNOMI_TENANT_ID:-default}"
SCOPE="${UNOMI_SCOPE:-default}"
PID="org.apache.unomi.samples.login"
SCRIPT_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
-SAMPLE_VERSION=""
usage() {
cat <<'USAGE'
Provisions and configures the Apache Unomi login sample on a local instance.
export UNOMI_ROOT_PASSWORD='your-admin-password'
- ./setup.sh [--version <sample-version>]
+ ./setup.sh
Environment overrides: UNOMI_URL, UNOMI_TENANT_ID, UNOMI_SCOPE, KARAF_HOME,
DEMO_PASSWORD.
USAGE
@@ -67,7 +66,6 @@ properties_escape() {
while [ $# -gt 0 ]; do
case "$1" in
- --version) SAMPLE_VERSION="${2:?--version needs a value}"; shift 2 ;;
-h|--help) usage; exit 0 ;;
*) echo "Unknown argument: $1 (try --help)" >&2; exit 2 ;;
esac
diff --git
a/samples/login-integration/src/main/java/org/apache/unomi/samples/login/LoginServlet.java
b/samples/login-integration/src/main/java/org/apache/unomi/samples/login/LoginServlet.java
index b603ce6ed..6618041b8 100644
---
a/samples/login-integration/src/main/java/org/apache/unomi/samples/login/LoginServlet.java
+++
b/samples/login-integration/src/main/java/org/apache/unomi/samples/login/LoginServlet.java
@@ -90,7 +90,7 @@ public class LoginServlet extends HttpServlet {
@ObjectClassDefinition(
name = "Unomi login sample",
- description = "Trusted credentials used by /login/authenticate to
call Unomi (UNOMI-972)"
+ description = "Trusted credentials used by /login/authenticate to
call Unomi (UNOMI-978)"
)
public @interface Config {
@@ -206,8 +206,12 @@ public class LoginServlet extends HttpServlet {
// would launder untrusted client input across the trust boundary and
let anyone who guesses
// another visitor's session id rebind or merge that visitor's
profile. Storing a generated
// id on the container's own HttpSession keeps it unreachable from the
page while staying stable
- // across requests from the same browser, which is what lets Unomi
recover the visitor's
- // pre-login anonymous profile.
+ // across requests from the same browser, so repeated logins from one
browser hit the same
+ // Unomi session. The trade-off is deliberate: nothing from the
visitor's own tracking
+ // (the context-profile-id cookie, the tracker's session id) is
forwarded, so the anonymous
+ // profile the browser built up before logging in is NOT linked here.
Linking it means
+ // forwarding a browser-chosen identifier under trusted credentials,
and an integration
+ // that wants that must first prove the identifier belongs to the
caller.
String sessionId = resolveSessionId(req);
// Only a tenant private key. A system administrator credential would
also satisfy the merge
diff --git
a/samples/login-integration/src/test/java/org/apache/unomi/samples/login/LoginServletTest.java
b/samples/login-integration/src/test/java/org/apache/unomi/samples/login/LoginServletTest.java
index 9a220fdc4..499ecb913 100644
---
a/samples/login-integration/src/test/java/org/apache/unomi/samples/login/LoginServletTest.java
+++
b/samples/login-integration/src/test/java/org/apache/unomi/samples/login/LoginServletTest.java
@@ -19,10 +19,22 @@ package org.apache.unomi.samples.login;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.sun.net.httpserver.HttpServer;
+
+import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
+import java.io.ByteArrayOutputStream;
+import java.io.InputStream;
import java.net.HttpURLConnection;
+import java.net.InetSocketAddress;
import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
@@ -350,4 +362,141 @@ class LoginServletTest {
private static int intThatIsAShortTimeout() {
return org.mockito.ArgumentMatchers.intThat(seconds -> seconds > 0 &&
seconds <= 600);
}
+
+ //
------------------------------------------------------------------------------------------
+ // doPost — the trusted call itself: right credentials, right event,
nothing from the caller
+ // beyond the form fields, and Unomi's cookies handed back to the browser.
+ //
------------------------------------------------------------------------------------------
+
+ @Test
+ @DisplayName("doPost calls Unomi with the configured tenant private key
and forwards the response and cookies")
+ void doPostCallsUnomiWithTrustedCredentialsAndForwardsCookies() throws
Exception {
+ AtomicReference<String> authorization = new AtomicReference<>();
+ AtomicReference<String> query = new AtomicReference<>();
+ AtomicReference<byte[]> body = new AtomicReference<>();
+ HttpServer unomi = HttpServer.create(new
InetSocketAddress("127.0.0.1", 0), 0);
+ unomi.createContext("/cxs/context.json", exchange -> {
+
authorization.set(exchange.getRequestHeaders().getFirst("Authorization"));
+ query.set(exchange.getRequestURI().getRawQuery());
+ try (InputStream in = exchange.getRequestBody()) {
+ body.set(in.readAllBytes());
+ }
+ byte[] out =
"{\"profileId\":\"master\"}".getBytes(StandardCharsets.UTF_8);
+ exchange.getResponseHeaders().add("Set-Cookie",
"context-profile-id=master; Path=/");
+ exchange.getResponseHeaders().add("Set-Cookie",
"context-session-id=s; Path=/");
+ exchange.getResponseHeaders().add("Content-Type",
"application/json");
+ exchange.sendResponseHeaders(200, out.length);
+ exchange.getResponseBody().write(out);
+ exchange.close();
+ });
+ unomi.start();
+ try {
+ LoginServlet servlet = new LoginServlet();
+ servlet.activate(config("http://127.0.0.1:"; +
unomi.getAddress().getPort(), "acme", "web", "private-key", "demo-pass"));
+
+ HttpSession session = statefulSession();
+ HttpServletRequest req = requestWithSession(session);
+ when(req.getParameter("email")).thenReturn("[email protected]");
+ when(req.getParameter("firstName")).thenReturn("Alice");
+ when(req.getParameter("lastName")).thenReturn("Doe");
+ when(req.getParameter("password")).thenReturn("demo-pass");
+
when(req.getParameter("sessionId")).thenReturn("attacker-chosen-session");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ ByteArrayOutputStream written = new ByteArrayOutputStream();
+ when(resp.getOutputStream()).thenReturn(capturing(written));
+
+ servlet.doPost(req, resp);
+
+ String expectedToken =
Base64.getEncoder().encodeToString("acme:private-key".getBytes(StandardCharsets.UTF_8));
+ assertEquals("Basic " + expectedToken, authorization.get(), "the
tenant private key must authenticate the call");
+ String unomiSessionId = (String)
session.getAttribute("org.apache.unomi.samples.login.unomiSessionId");
+ assertEquals("sessionId=" + unomiSessionId, query.get(), "the
session id must be the servlet's own, never the caller's");
+ JsonNode sent = new ObjectMapper().readTree(body.get());
+ assertEquals("login",
sent.get("events").get(0).get("eventType").asText());
+ assertEquals("web",
sent.get("events").get(0).get("scope").asText());
+ assertEquals("[email protected]",
sent.get("events").get(0).get("target").get("properties").get("email").asText());
+ verify(resp).setStatus(200);
+ verify(resp).addHeader("Set-Cookie", "context-profile-id=master;
Path=/");
+ verify(resp).addHeader("Set-Cookie", "context-session-id=s;
Path=/");
+ assertEquals("master", new
ObjectMapper().readTree(written.toByteArray()).get("profileId").asText());
+ } finally {
+ unomi.stop(0);
+ }
+ }
+
+ @Test
+ @DisplayName("doPost rejects a wrong demo password before touching Unomi")
+ void doPostRejectsWrongPasswordWithoutCallingUnomi() throws Exception {
+ AtomicReference<Boolean> called = new AtomicReference<>(false);
+ HttpServer unomi = HttpServer.create(new
InetSocketAddress("127.0.0.1", 0), 0);
+ unomi.createContext("/", exchange -> { called.set(true);
exchange.sendResponseHeaders(500, -1); exchange.close(); });
+ unomi.start();
+ try {
+ LoginServlet servlet = new LoginServlet();
+ servlet.activate(config("http://127.0.0.1:"; +
unomi.getAddress().getPort(), "acme", "web", "private-key", "demo-pass"));
+ HttpServletRequest req = requestWithSession(statefulSession());
+ when(req.getParameter("email")).thenReturn("[email protected]");
+ when(req.getParameter("password")).thenReturn("wrong");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(resp.getOutputStream()).thenReturn(capturing(new
ByteArrayOutputStream()));
+
+ servlet.doPost(req, resp);
+
+ verify(resp).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ assertFalse(called.get(), "Unomi must not be called for a failed
login");
+ } finally {
+ unomi.stop(0);
+ }
+ }
+
+ @Test
+ @DisplayName("doPost answers 502 with a generic message when Unomi is
unreachable")
+ void doPostAnswersBadGatewayWhenUnomiIsUnreachable() throws Exception {
+ HttpServer probe = HttpServer.create(new
InetSocketAddress("127.0.0.1", 0), 0);
+ int closedPort = probe.getAddress().getPort();
+ probe.stop(0);
+ LoginServlet servlet = new LoginServlet();
+ servlet.activate(config("http://127.0.0.1:"; + closedPort, "acme",
"web", "private-key", "demo-pass"));
+ HttpServletRequest req = requestWithSession(statefulSession());
+ when(req.getParameter("email")).thenReturn("[email protected]");
+ when(req.getParameter("password")).thenReturn("demo-pass");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ ByteArrayOutputStream written = new ByteArrayOutputStream();
+ when(resp.getOutputStream()).thenReturn(capturing(written));
+
+ servlet.doPost(req, resp);
+
+ verify(resp).setStatus(HttpServletResponse.SC_BAD_GATEWAY);
+ String message = written.toString(StandardCharsets.UTF_8);
+ assertTrue(message.contains("Profile service unavailable"), message);
+ assertFalse(message.contains("127.0.0.1"), "the upstream address must
not leak: " + message);
+ }
+
+ private static LoginServlet.Config config(String baseUrl, String tenantId,
String scope, String privateKey, String demoPassword) {
+ LoginServlet.Config config = mock(LoginServlet.Config.class);
+ when(config.unomiBaseUrl()).thenReturn(baseUrl);
+ when(config.tenantId()).thenReturn(tenantId);
+ when(config.scope()).thenReturn(scope);
+ when(config.privateKey()).thenReturn(privateKey);
+ when(config.demoPassword()).thenReturn(demoPassword);
+ return config;
+ }
+
+ private static ServletOutputStream capturing(ByteArrayOutputStream sink) {
+ return new ServletOutputStream() {
+ @Override
+ public void write(int b) {
+ sink.write(b);
+ }
+
+ @Override
+ public boolean isReady() {
+ return true;
+ }
+
+ @Override
+ public void setWriteListener(javax.servlet.WriteListener
writeListener) {
+ }
+ };
+ }
}
