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

PATCH for groups returns 500 when members are updated #731

Closed antifree closed 3 weeks ago

antifree commented 2 months ago

Hello team!

I faced strange issue. For some groups on SCIM PATCH server returns 500 error.

In logs the following error is shown: 5 rows were copied to scim.SCIMRepresentationAttributeLst but only 2 were inserted.

The detailed stack trace:

MySqlConnector.MySqlException (0x80004005): 5 rows were copied to scim.SCIMRepresentationAttributeLst but only 2 were inserted. at MySqlConnector.MySqlBulkCopy.WriteToServerAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in //src/MySqlConnector/MySqlBulkCopy.cs:line 347 at MySqlConnector.MySqlBulkCopy.WriteToServerAsync(DataTable dataTable, CancellationToken cancellationToken) in //src/MySqlConnector/MySqlBulkCopy.cs:line 148 at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, Boolean isAsync, CancellationToken cancellationToken) at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, Boolean isAsync, CancellationToken cancellationToken) at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, CancellationToken cancellationToken) at EFCore.BulkExtensions.SqlBulkOperation.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, CancellationToken cancellationToken) at EFCore.BulkExtensions.DbContextBulkTransaction.ExecuteAsync[T](DbContext context, Type type, IEnumerable1 entities, OperationType operationType, BulkConfig bulkConfig, Action1 progress, CancellationToken cancellationToken) at SimpleIdServer.Scim.Commands.Handlers.PatchRepresentationCommandHandler.UpdateRepresentation(SCIMRepresentation existingRepresentation, PatchRepresentationCommand patchRepresentationCommand, SCIMSchema schema) at SimpleIdServer.Scim.Commands.Handlers.PatchRepresentationCommandHandler.UpdateRepresentation(SCIMRepresentation existingRepresentation, PatchRepresentationCommand patchRepresentationCommand, SCIMSchema schema)

SCIM Provider: Azure AD Database: MySQL 8.0 (AWS Aurora MySQL to be specific) Entity Framework Provider: Pomelo.EntityFrameworkCore.MySql SimpleIdServer Version: 4.0.8

Could you please take a look at it?

Please let me know, if more information should be provided.

simpleidserver commented 2 months ago

I quickly checked the Nuget Package EFCore.BulkExtensions, which is used by SimpleIdServer for bulk data insertion. It appears to utilize the Nuget Package MySQLConnector. According to Ticket Ticket #1145, the Nuget package lacks sufficient information to diagnose the data issue causing the errors.

In the EFSCIMRepresentationCommandRepository class, could you please update the methods BulkInsert, BulkDelete, and BulkUpdate to capture the error messages originating from the MySQLConnection?

The code should resemble something like this:

var connection = context.Database.GetDbConnection() as MySqlConnection;
connection.InfoMessage += (s, e) =>
{
    // use logging infrastructure of your choice
    foreach (var error in e.Errors)
        Console.WriteLine(error.Message);
};

If this is too challenging, could you monitor and provide the incoming HTTP request that triggers this issue? I will attempt to reproduce the problem.

antifree commented 2 months ago

Hi @simpleidserver !

Thanks a lot for the recommendation!

I spent some time previously to set up test environment local to reproduce the issue and I included library directly.

So the error it the following:

Cannot add or update a child row: a foreign key constraint fails (`scim`.`SCIMRepresentationAttributeLst`, CONSTRAINT `FK_SCIMRepresentationAttributeLst_SCIMRepresentationAttributeLs~` FOREIGN KEY (`ParentAttributeId`) REFERENCES `SCIMRepresentationAttribute)

I assume it is related to the order of bulk insert:

As you can see, the list of inserted attributes is the following: image

It seems the groups attribute itself is the last in groups.* sub attributes set.

I think groups and members.type attributes are set successfully, but sub attributes of user's groups property fails because it tries to insert it before.

P.s.

var attributesToInsert = scimRepresentationAttributes.ToList();
antifree commented 2 months ago

So, i've made this small hack to test my theory:

var attributesToInsert = scimRepresentationAttributes.OrderBy(x => x.ParentAttributeId).ToList();

It placed groups before related sub-properties.

And it worked.

If you could suggest me, where is better to fix ordering, I could prepare an MR with more correct fix

simpleidserver commented 2 months ago

Thank you for the information :)

I'll take a look and fix this issue for the next release!

simpleidserver commented 2 months ago

Great!

You can submit the pull request with this fix :)

antifree commented 2 months ago

So, I think I found where it is better to make this fix.

I think in RepresentationReferenceSync.BuildScimRepresentationAttribute we should return parent not the last, but the first.

I can prepare MR a little bit later

simpleidserver commented 3 weeks ago

This issue is fixed in the version 5.0.0