Check for property fields first before handling any native fields, in case of a name collision

server/channels/app/access_control.go

@@ -1778,24 +1778,38 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, resourceID stri
 		return map[string][]string{}, nil
 	}
 
-	// We will be looking up field names by the store, but first we need to check for any synthetic
-	// native fields.  At the time of writing this comment, all native fields are public, so
+	// Native fields won't show up in the Property Store, since they are synthetic (generated in code).
+	// At the time of writing this comment, all native fields are public, so
 	// we could just check for each name and give it a pass without building the array, but this
 	// approach will make sure any potential future native fields can also support different AccessModes.
+
+	// We will check for attributes in the store first because there is a known potential for name collision
+	// (i.e. if an admin makes an access control property with the same name as a native attribute).
+	// The collision doesn't happen during storage since there are unqiue prefixes, but the attributes
+	// returned by GetPolicyRuleAttributes are stripped of those prefixes, leaving it ambiguous.
+	// While that should be addressed, for now we check for real properties first and then fall back to native fields.
 	nativeFieldsByName := make(map[string]*model.PropertyField)
 	for _, f := range model.NativeUserAttributeFields(cpaGroup.ID) {
 		nativeFieldsByName[f.Name] = f
 	}
 
 	for fieldName := range attributes {
-		// Check if this is a (synthetic) native field before reading from the store
-		field, ok := nativeFieldsByName[fieldName]
-		if !ok {
-			// Read directly from the store so this security filter sees the raw
-			// access_mode, unaffected by property read hooks for the request caller.
-			var fieldErr error
-			field, fieldErr = a.Srv().Store().PropertyField().GetFieldByNameForObjectType(rctx, cpaGroup.ID, "", model.PropertyFieldObjectTypeUser, fieldName)
-			if fieldErr != nil {
+		// Read directly from the store so this security filter sees the raw
+		// access_mode, unaffected by property read hooks for the request caller.
+		field, fieldErr := a.Srv().Store().PropertyField().GetFieldByNameForObjectType(rctx, cpaGroup.ID, "", model.PropertyFieldObjectTypeUser, fieldName)
+		if fieldErr != nil {
+			// If the error is due to not being found, we won't skip to the next field just yet
+			// in case it is a native field
+			var nfErr *store.ErrNotFound
+			notFound := errors.As(fieldErr, &nfErr)
+			if !notFound {
+				delete(attributes, fieldName)
+				continue
+			}
+
+			//If property wasn't found, check if this is a native field.
+			field = nativeFieldsByName[fieldName]
+			if field == nil {
 				delete(attributes, fieldName)
 				continue
 			}

server/channels/app/access_control_test.go

@@ -4790,7 +4790,7 @@ func TestGetAccessControlPolicyAttributes_MaskedFieldsFiltered(t *testing.T) {
 	mockACS := &mocks.AccessControlServiceInterface{}
 	th.App.Srv().ch.AccessControl = mockACS
 	mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership).
-		Return(rawAttributes, nil).Once()
+		Return(maps.Clone(rawAttributes), nil).Once()
 
 	result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership)
 	require.Nil(t, appErr)
@@ -4865,6 +4865,74 @@ func TestGetAccessControlPolicyAttributes_NativeFieldsPassThrough(t *testing.T)
 	mockACS.AssertExpectations(t)
 }
 
+// TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered checks that
+// a masked field that happens to share the same name as a native filed stil gets filtered.
+func TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered(t *testing.T) {
+	th := Setup(t).InitBasic(t)
+	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise))
+
+	rctx := request.TestContext(t)
+
+	cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName)
+	require.Nil(t, cErr)
+
+	permNone := model.PermissionLevelNone
+
+	makeField := func(name, accessMode string) {
+		protected := accessMode == model.PropertyAccessModeSourceOnly || accessMode == model.PropertyAccessModeSharedOnly
+		f := &model.PropertyField{
+			GroupID:    cpaGroup.ID,
+			Name:       name,
+			Type:       model.PropertyFieldTypeText,
+			ObjectType: model.PropertyFieldObjectTypeUser,
+			TargetType: string(model.PropertyFieldTargetLevelSystem),
+			Protected:  protected,
+			Attrs:      model.StringInterface{model.PropertyAttrsAccessMode: accessMode},
+		}
+		if protected {
+			f.PermissionField = &permNone
+			f.Attrs[model.PropertyAttrsProtected] = true
+			_, err := th.App.Srv().Store().PropertyField().Create(f)
+			require.NoError(t, err)
+		} else {
+			_, appErr := th.App.CreatePropertyField(rctx, f, false, "")
+			require.Nil(t, appErr)
+		}
+	}
+
+	makeField("PublicField", model.PropertyAccessModePublic)
+	makeField("SourceField", model.PropertyAccessModeSourceOnly)
+	// This is the field with a native name that we are testing to make sure it is still masked.
+	makeField(model.NativeAttributePropertyFieldIsBot, model.PropertyAccessModeSharedOnly)
+
+	channelID := model.NewId()
+	rawAttributes := map[string][]string{
+		"PublicField":                           {"Engineering"},
+		"SourceField":                           {"TopSecret"},
+		model.NativeAttributePropertyFieldIsBot: {"false"},
+		// This is a real native attribute that is not in the property store -it should pass through.
+		model.NativeAttributePropertyFieldVerified: {"true"},
+	}
+
+	mockACS := &mocks.AccessControlServiceInterface{}
+	th.App.Srv().ch.AccessControl = mockACS
+	mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership).
+		Return(rawAttributes, nil).Once()
+
+	result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership)
+	require.Nil(t, appErr)
+
+	// Only the public field should survive.
+	expectedAttributes := map[string][]string{
+		"PublicField": {"Engineering"},
+		model.NativeAttributePropertyFieldVerified: {"true"},
+	}
+	assert.Equal(t, expectedAttributes, result)
+	assert.NotContains(t, result, "SourceField")
+	assert.NotContains(t, result, model.NativeAttributePropertyFieldIsBot)
+	mockACS.AssertExpectations(t)
+}
+
 // TestMergeStoredPolicyExpressions_ActionsLocked verifies that a caller who
 // cannot see all values in a stored rule cannot change that rule's Actions.
 // The attack: submit a PUT with the same masked expression but a different