Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class DataplanBlockingUserTests : BaseKitOptionsTest() {
assertTrue(allowedAttributes.containsKey(key))
assertFalse(blockedAttributes.containsKey(key))
}
attributeListenerKitKit.setUserAttribute = { key, _ ->
attributeListenerKitKit.setUserAttributeCallback = { key, _ ->
assertTrue(allowedAttributes.containsKey(key))
assertFalse(blockedAttributes.containsKey(key))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ open class AttributeListenerTestKit :
ListenerTestKit(),
AttributeListener,
LogoutListener {
var setUserAttribute: ((attributeKey: String?, attributeValue: String?) -> Unit)? = null
var setUserAttributeCallback: ((attributeKey: String?, attributeValue: String?) -> Unit)? = null
var setUserAttributeList: ((attributeKey: String?, attributeValueList: List<String?>?) -> Unit)? =
null
var supportsAttributeLists: (() -> Boolean)? = null
Expand Down Expand Up @@ -41,14 +41,6 @@ open class AttributeListenerTestKit :
userAttributeLists.forEach { onAttributeReceived?.invoke(it.key, it.value) }
}

override fun setUserAttribute(
attributeKey: String,
attributeValue: String?,
) {
setUserAttribute?.invoke(attributeKey, attributeValue)
onAttributeReceived?.invoke(attributeKey, attributeValue)
}

override fun setUserIdentity(
identityType: MParticle.IdentityType,
identity: String?,
Expand All @@ -70,5 +62,17 @@ open class AttributeListenerTestKit :
onAttributeReceived?.invoke(key, null)
}

override fun onSetUserAttribute(
key: String?,
value: Any?,
user: FilteredMParticleUser?,
) {
if (key == null || value == null || value !is String) {
return
}
setUserAttributeCallback?.invoke(key, value)
onAttributeReceived?.invoke(key, value)
}

override fun logout(): List<ReportingMessage> = logout?.invoke() ?: listOf()
}
Original file line number Diff line number Diff line change
Expand Up @@ -387,12 +387,19 @@ public interface BaseAttributeListener {
* @param user filtered user context for this kit
*/
void onRemoveUserAttribute(String key, FilteredMParticleUser user);

/**
* Called when a scalar user attribute is set for the current user.
*
* @param key attribute key
* @param value attribute value (may be non-String for some {@link UserAttributeListener} call paths)
* @param user filtered user context for this kit
*/
void onSetUserAttribute(String key, Object value, FilteredMParticleUser user);
}

public interface AttributeListener extends BaseAttributeListener {

void setUserAttribute(String attributeKey, String attributeValue);

void setUserAttributeList(String attributeKey, List<String> attributeValueList);

void setAllUserAttributes(Map<String, String> userAttributes, Map<String, List<String>> userAttributeLists);
Expand Down Expand Up @@ -556,8 +563,6 @@ public interface UserAttributeListener extends BaseAttributeListener {

void onIncrementUserAttribute(String key, Number incrementedBy, String value, FilteredMParticleUser user);

void onSetUserAttribute(String key, Object value, FilteredMParticleUser user);

void onSetUserTag(String key, FilteredMParticleUser user);

void onSetUserAttributeList(String attributeKey, List<String> attributeValueList, FilteredMParticleUser user);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,34 +685,30 @@ private void setUserAttribute(KitIntegration provider, String attributeKey, List
&& !provider.isDisabled()
&& KitConfiguration.shouldForwardAttribute(provider.getConfiguration().getUserAttributeFilters(), attributeKey)) {
boolean supportsAttributeLists = ((KitIntegration.BaseAttributeListener) provider).supportsAttributeLists();
FilteredMParticleUser user = FilteredMParticleUser.getInstance(mpid, provider);
if (provider instanceof KitIntegration.AttributeListener) {
if (supportsAttributeLists) {
((KitIntegration.AttributeListener) provider).setUserAttributeList(attributeKey, valueList);
} else {
((KitIntegration.AttributeListener) provider).setUserAttribute(attributeKey, KitUtils.join(valueList));
((KitIntegration.BaseAttributeListener) provider).onSetUserAttribute(attributeKey, KitUtils.join(valueList), user);
}
}
if (provider instanceof KitIntegration.UserAttributeListener) {
if (supportsAttributeLists) {
((KitIntegration.UserAttributeListener) provider).onSetUserAttributeList(attributeKey, valueList, FilteredMParticleUser.getInstance(mpid, provider));
((KitIntegration.UserAttributeListener) provider).onSetUserAttributeList(attributeKey, valueList, user);
} else {
((KitIntegration.UserAttributeListener) provider).onSetUserAttribute(attributeKey, KitUtils.join(valueList), FilteredMParticleUser.getInstance(mpid, provider));
((KitIntegration.UserAttributeListener) provider).onSetUserAttribute(attributeKey, KitUtils.join(valueList), user);
Comment thread
denischilik marked this conversation as resolved.
}
}
}
}

private void setUserAttribute(KitIntegration provider, String attributeKey, String attributeValue, long mpid) {
if ((provider instanceof KitIntegration.AttributeListener || provider instanceof KitIntegration.UserAttributeListener)
if ((provider instanceof KitIntegration.BaseAttributeListener listener)
&& !provider.isDisabled()
&& KitConfiguration.shouldForwardAttribute(provider.getConfiguration().getUserAttributeFilters(),
attributeKey)) {
if (provider instanceof KitIntegration.AttributeListener) {
((KitIntegration.AttributeListener) provider).setUserAttribute(attributeKey, attributeValue);
}
if (provider instanceof KitIntegration.UserAttributeListener) {
((KitIntegration.UserAttributeListener) provider).onSetUserAttribute(attributeKey, attributeValue, FilteredMParticleUser.getInstance(mpid, provider));
}
listener.onSetUserAttribute(attributeKey, attributeValue, FilteredMParticleUser.getInstance(mpid, provider));
}
}

Expand Down Expand Up @@ -745,8 +741,8 @@ public void incrementUserAttribute(String key, Number incrementedBy, String newV
if (provider instanceof KitIntegration.UserAttributeListener) {
((KitIntegration.UserAttributeListener) provider).onIncrementUserAttribute(key, incrementedBy, newValue, FilteredMParticleUser.getInstance(mpid, provider));
}
if (provider instanceof KitIntegration.AttributeListener) {
((KitIntegration.AttributeListener) provider).setUserAttribute(key, newValue);
if (provider instanceof KitIntegration.BaseAttributeListener listener) {
listener.onSetUserAttribute(key, newValue, FilteredMParticleUser.getInstance(mpid, provider));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

onSetUserAttribute bypasses disabled and filter guards in increment

High Severity

In incrementUserAttribute, the onSetUserAttribute call on BaseAttributeListener at line 743 sits outside the braceless if (!provider.isDisabled() && shouldForwardAttribute(...)) guard at line 739. That guard only controls the nested onIncrementUserAttribute call. As a result, onSetUserAttribute is dispatched to every BaseAttributeListener kit regardless of whether the provider is disabled or the attribute is filtered. Compare this to the properly guarded setUserAttribute(provider, key, value, mpid) private helper and removeUserAttribute, which both wrap their calls inside the full check. Additionally, the old code scoped this unguarded path to AttributeListener only, but the change to BaseAttributeListener now also dispatches to all UserAttributeListener kits (e.g. Braze, Singular), which never received this call during increments before.

Additional Locations (1)
Fix in Cursor Fix in Web

}
} catch (Exception e) {
Logger.warning("Failed to call onIncrementUserAttribute for kit: " + provider.getName() + ": " + e.getMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.mparticle.internal.Logger
import com.mparticle.internal.MPUtility
import com.mparticle.internal.SideloadedKit
import com.mparticle.kits.KitIntegration.AttributeListener
import com.mparticle.kits.KitIntegration.BaseAttributeListener
import com.mparticle.mock.MockContext
import com.mparticle.mock.MockKitConfiguration
import com.mparticle.mock.MockKitManagerImpl
Expand All @@ -49,6 +50,8 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentCaptor
import org.mockito.ArgumentMatchers.any
import org.mockito.ArgumentMatchers.eq
import org.mockito.ArgumentMatchers.isNull
import org.mockito.Mockito
import org.mockito.Mockito.mock
import org.mockito.Mockito.never
Expand Down Expand Up @@ -557,8 +560,8 @@ class KitManagerImplTest {
manager.setUserAttributeList("test key", attributeList, 1)
verify(integration as AttributeListener, Mockito.times(1))
.setUserAttributeList("test key", attributeList)
verify(integration2 as AttributeListener, Mockito.times(1))
.setUserAttribute("test key", "1,2,3")
verify(integration2 as BaseAttributeListener, Mockito.times(1))
.onSetUserAttribute(eq("test key"), eq("1,2,3"), isNull())
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,6 @@ abstract class AdobeKitBase :
syncIds()
}

override fun setUserAttribute(
s: String,
s1: String,
) {
syncIds()
}

override fun setUserAttributeList(
s: String,
list: List<String>,
Expand All @@ -78,6 +71,17 @@ abstract class AdobeKitBase :
syncIds()
}

override fun onSetUserAttribute(
key: String?,
value: Any?,
user: FilteredMParticleUser?,
) {
if (key == null || value == null || value !is String) {
return
}
syncIds()
}

override fun setUserIdentity(
identityType: IdentityType,
s: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,6 @@ open class AdobeKit :
syncIds()
}

override fun setUserAttribute(
s: String,
s1: String,
) {
syncIds()
}

override fun setUserAttributeList(
s: String,
list: List<String>,
Expand All @@ -115,6 +108,17 @@ open class AdobeKit :
syncIds()
}

override fun onSetUserAttribute(
key: String?,
value: Any?,
user: FilteredMParticleUser?,
) {
if (key == null || value == null || value !is String) {
return
}
syncIds()
}

override fun setUserIdentity(
identityType: MParticle.IdentityType,
s: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,6 @@ class AppsFlyerKit :
return messageList
}

override fun setUserAttribute(
attributeKey: String,
attributeValue: String,
) {}

override fun setUserAttributeList(
s: String,
list: List<String>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,6 @@ class ApptimizeKit :

override fun getName(): String = KIT_NAME

override fun setUserAttribute(
key: String,
value: String,
) {
Apptimize.setUserAttribute(key, value)
}

/**
* Not supported by the Apptimize kit.
*/
Expand All @@ -148,7 +141,7 @@ class ApptimizeKit :
attributeLists: Map<String, List<String>>,
) {
for ((key, value) in attributes) {
setUserAttribute(key, value)
Apptimize.setUserAttribute(key, value)
}
}

Expand All @@ -159,6 +152,17 @@ class ApptimizeKit :
Apptimize.clearUserAttribute(key)
}

override fun onSetUserAttribute(
key: String?,
value: Any?,
user: FilteredMParticleUser?,
) {
if (key == null || value == null || value !is String) {
return
}
Apptimize.setUserAttribute(key, value)
}

/**
* @param identityType only Alias and CustomerId are suppoted by the Apptimize kit.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,6 @@ class BranchMetricsKit :
)
}

override fun setUserAttribute(
s: String,
s1: String,
) {}

override fun setUserAttributeList(
s: String,
list: List<String>,
Expand All @@ -198,6 +193,14 @@ class BranchMetricsKit :
// No-op: this kit does not implement this feature.
}

override fun onSetUserAttribute(
key: String?,
value: Any?,
user: FilteredMParticleUser?,
) {
// No-op: this kit does not implement this feature.
}

override fun setUserIdentity(
identityType: IdentityType,
s: String,
Expand Down
Loading
Loading