simpleidserver / SimpleIdServer

OpenID, OAuth 2.0, SCIM2.0, UMA2.0, FAPI, CIBA & OPENBANKING Framework for ASP.NET Core
https://simpleidserver.com/
Apache License 2.0
682 stars 90 forks source link

Bulk updates only change string columns #735

Closed RobTF closed 3 weeks ago

RobTF commented 2 months ago

Hi,

When testing SCIM functionality with Entra ID (formally Azure AD) I noticed that when a user is disabled their active flag never goes back to "0" in the "ValueBoolean" column.

This appears to be a bug in the BulkUpdate method in EFSCIMRepresentationCommandRepository where at some point only two columns are named to be part of the bulk update;

Notably line 226 of EFSCIMRepresentationCommandRepository.cs where the bulkConfig variable is initialized.

        public async Task BulkUpdate(IEnumerable<SCIMRepresentationAttribute> scimRepresentationAttributes, bool isReference = false)
        {
            scimRepresentationAttributes = scimRepresentationAttributes.Where(r => !string.IsNullOrWhiteSpace(r.RepresentationId));
            foreach (var attr in scimRepresentationAttributes)
                attr.SchemaAttributeId = attr.SchemaAttribute?.Id;

            // **************************************************************************************
            // Why only ValueString and ComputedValueIndex????
            var bulkConfig = new BulkConfig
            {
                PropertiesToInclude = new List<string> { nameof(SCIMRepresentationAttribute.ValueString), nameof(SCIMRepresentationAttribute.ComputedValueIndex) }
            };
            // ***************************************************************************************

            bulkConfig = GetBulkConfig(BulkOperations.UPDATE, bulkConfig);
            await _scimDbContext.BulkUpdateAsync(scimRepresentationAttributes.ToList(), bulkConfig);
        }

If I add nameof(SCIMRepresentationAttribute.ValueBoolean) to the list of properties the active attribute is set successfully.

Is this a simple oversight? Is there any other implication to adding more properties to this list?

thanks

simpleidserver commented 2 months ago

Hello,

Firstly, thank you for conducting the investigation :)

Indeed, there is an error in the BulkUpdate operation of the EFSCIMRepresentationCommandRepository class. All properties that begin with the name Value and ComputedValueIndex must be added to the PropertiesToInclude list.

I have made some modifications in the master branch to address this issue.

Kind regards,

SID

RobTF commented 2 months ago

Great, thanks for the confirmation!

simpleidserver commented 3 weeks ago

This issue is fixed in the version 5.0.0