Standard schema bugs - Githubissues

canea-asb commented 3 months ago

I think that I have found two bugs in SimpleIdServer.Scim.Domain. Perhaps they are intentional and if so please explain why the code is like this. Just trying to help out. :)

Do this before reproducing the bugs

Replace the content of Program.cs with the following:


// Copyright (c) SimpleIdServer. All rights reserved.
// Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information.
using AspNetCore.Authentication.ApiKey;
using MassTransit;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using MongoDB.Driver;
using SimpleIdServer.Scim.Domains;
using SimpleIdServer.Scim.Infrastructure;
using SimpleIdServer.Scim.Persistence.MongoDB.Extensions;
using SimpleIdServer.Scim.Persistence.MongoDB.Infrastructures;
using SimpleIdServer.Scim.Persistence.MongoDB.Models;
using SimpleIdServer.Scim.Startup.Configurations;
using SimpleIdServer.Scim.Startup.Consumers;
using SimpleIdServer.Scim.Startup.Services;
using SimpleIdServer.Scim.SwashbuckleV6;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Reflection;

namespace SimpleIdServer.Scim.Startup;

public class Program
{
    private const string ApiTitle = "SCIM API";
    private const int ApiVersion = 1;
    private static readonly string ApiName = $"{ApiTitle} v{ApiVersion}";
    private const bool UsePrefix = false; // e.g. if true we use "scim/v2/Users" instead of just "Users"

    public static void Main(string[] args)
    {
        var builder = WebApplication.CreateBuilder(args);
        builder.Configuration
            .AddJsonFile(path: "appsettings.json", optional: false, reloadOnChange: true)
            .AddJsonFile(path: $"appsettings.{builder.Environment.EnvironmentName}.json", optional: true, reloadOnChange: true)
            .AddEnvironmentVariables();
        ConfigureServices(builder);
        var app = builder.Build();
        var environmentName = app.Environment.EnvironmentName;
        app.Logger.LogInformation("Starting {ApiName} in {EnvironmentName} environment" , ApiName, environmentName);
        ConfigureApp(builder, app);
        app.Run();
    }

    private static void ConfigureServices(WebApplicationBuilder builder)
    {
        if (builder.Environment.IsDevelopment())
        {
            builder.Services.AddControllers(); // This ensures that AuthController can be used in development
        }

        builder.Services.AddMvc(o =>
        {
            o.EnableEndpointRouting = false;
            o.AddSCIMValueProviders();
        }).AddNewtonsoftJson();
        builder.Services.AddLogging();

        // Turn off authorization for demonstration purposes
        builder.Services.AddAuthorization(opts =>
        {
            opts.AddPolicy("QueryScimResource", p => p.RequireAssertion(_ => true));
            opts.AddPolicy("AddScimResource", p => p.RequireAssertion(_ => true));
            opts.AddPolicy("DeleteScimResource", p => p.RequireAssertion(_ => true));
            opts.AddPolicy("UpdateScimResource", p => p.RequireAssertion(_ => true));
            opts.AddPolicy("BulkScimResource", p => p.RequireAssertion(_ => true));
        });

        ConfigureScim(builder);
        builder.Services.Configure<RouteOptions>(opt =>
        {
            opt.ConstraintMap.Add("realmPrefix", typeof(RealmRoutePrefixConstraint));
        });
    }

    #region Dependency injection

    private static void ConfigureScim(WebApplicationBuilder builder)
    {
        builder.Services.AddSIDScim(o =>
        {
            o.IgnoreUnsupportedCanonicalValues = false;
            o.EnableRealm = bool.Parse(builder.Configuration["IsRealmEnabled"] ?? "false");
            o.IsFullRepresentationReturned = true;
        }, massTransitOptions: c =>
        {
            // SimpleIdServer uses a popular library called MassTransit (https://masstransit.io/) to send messages to the bus.
            // Basically, the configuration below adds a consumer to messages that SimpleIdServer sends to the bus.
            // It adds hooks that we can use to integrate with SimpleIdServer.
            // These hooks call the user management API of CANEA ONE.
            c.AddConsumer<IntegrationEventConsumer>();

            // The configuration below is for the "in-memory" transport.
            // It is used for testing and development, not for production!
            // Read about the "in-memory" transport here: https://masstransit.io/documentation/transports/in-memory
            c.UsingInMemory((context, cfg) =>
            {
                cfg.ConfigureEndpoints(context);
            });
        });
    }

    #endregion

    private static void ConfigureApp(WebApplicationBuilder builder, WebApplication app)
    {
        var opts = app.Services.GetRequiredService<IOptions<SCIMHostOptions>>().Value;

        // Add the prefix /scim/v2 to all routes (because UsePrefix did not work when I tried it in the call to UseScim)
        // Source: https://stackoverflow.com/questions/58340979/how-to-add-global-route-prefix-in-asp-net-core-3
        app.UsePathBase(new PathString("/scim/v2"));
        app.UseRouting();

        // Notice that we have no SCIM database for persistence
        // No auth either because this is just for demonstration

        app.UseMvc(e =>
        {
            e.UseScim(opts.EnableRealm || UsePrefix);
        });

        if (app.Environment.IsDevelopment())
        {
            app.UseDeveloperExceptionPage();
            app.MapControllers();
        }
    }
}

Run the Startup project now.

Bug 1: Cannot specify the attribute "primary" in an address.

Given that you are running the Program.cs file above... When sending the HTTP request below:

POST https://localhost:5003/scim/v2/users
content-type: application/scim+json

{
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User"
    ],
    "userName": "user3",
    "externalId": "adUser3",
    "name": {
        "familyName": "Doe",
        "givenName": "John"
    },
    "addresses": [
        {
            "streetAddress": "100 Universal City Plaza",
            "locality": "Hollywood",
            "region": "CA",
            "postalCode": "91608",
            "country": "USA",
            "formatted": "100 Universal City Plaza\nHollywood, CA 91608 USA",
            "type": "work",
            "primary": true
        }
    ]
}

Then you get the following exception:

fail: SimpleIdServer.Scim.Api.UsersController[0]
      attribute primary is not recognized by the SCIM schema
      SimpleIdServer.Scim.Exceptions.SCIMSchemaViolatedException: attribute primary is not recognized by the SCIM schema
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.Resolve(KeyValuePair`2 kvp, SCIMSchema schema, ICollection`1 schemaAttributes) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 926
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.Resolve(JObject json, SCIMSchema schema, ICollection`1 schemaAttributes) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 880
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.BuildAttributes(JArray jArr, SCIMSchemaAttribute schemaAttribute, SCIMSchema schema, Boolean ignoreUnsupportedCanonicalValues) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 711
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.BuildRepresentationAttributes(ResolutionResult resolutionResult, ICollection`1 allSchemaAttributes, Boolean ignoreUnsupportedCanonicalValues, Boolean ignoreDefaultAttrs) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 625
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.BuildRepresentation(JObject json, String externalId, SCIMSchema mainSchema, ICollection`1 extensionSchemas, Boolean ignoreUnsupportedCanonicalValues) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 595
         at SimpleIdServer.Scim.Helpers.RepresentationHelper.ExtractSCIMRepresentationFromJSON(JObject json, String externalId, SCIMSchema mainSchema, ICollection`1 extensionSchemas) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Helpers\RepresentationHelper.cs:line 578
         at SimpleIdServer.Scim.Commands.Handlers.AddRepresentationCommandHandler.Handle(AddRepresentationCommand addRepresentationCommand) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Commands\Handlers\AddRepresentationCommandHandler.cs:line 60
         at SimpleIdServer.Scim.Api.BaseApiController.InternalAdd(String prefix, RepresentationParameter jobj, CancellationToken cancellationToken) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Api\BaseApiController.cs:line 354

It can be solved by adding c.AddBooleanAttribute("primary"); after line 81 in StandardSchemas.cs in SimpleIdServer.Scim.Domains.StandardSchemas (see the commits in this PR). Why do I think this is a bug? Because according to RFC 7643 it is possible to specify "primary" for an address (see the example here).

I think it is appropriate to also update the UserSchema.json in the Schemas directory of SimpeIdServer.Scim.Startup to reflect this change.

Bug 2: Crash when sending POST to /Users

Given that you are running the Program.cs file above... When sending a POST request like this (replace Bearer token in the HTTP Authorization header):

POST https://localhost:5003/scim/v2/users
content-type: application/scim+json

{
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User"
    ],
    "userName": "user3",
    "externalId": "adUser3",
    "name": {
        "familyName": "Doe",
        "givenName": "John"
    }
}

Then you get the following exception:

info: SimpleIdServer.Scim.Api.UsersController[0]
      add resource
fail: SimpleIdServer.Scim.Api.UsersController[0]
      Can not add property groups to Newtonsoft.Json.Linq.JObject. Property with the same name already exists on object.
      System.ArgumentException: Can not add property groups to Newtonsoft.Json.Linq.JObject. Property with the same name already exists on object.
         at Newtonsoft.Json.Linq.JObject.ValidateToken(JToken o, JToken existing)
         at Newtonsoft.Json.Linq.JContainer.InsertItem(Int32 index, JToken item, Boolean skipParentCheck, Boolean copyAnnotations)
         at Newtonsoft.Json.Linq.JObject.InsertItem(Int32 index, JToken item, Boolean skipParentCheck, Boolean copyAnnotations)
         at Newtonsoft.Json.Linq.JContainer.TryAddInternal(Int32 index, Object content, Boolean skipParentCheck, Boolean copyAnnotations)
         at Newtonsoft.Json.Linq.JContainer.Add(Object content)
         at Newtonsoft.Json.Linq.JObject.Add(String propertyName, JToken value)
         at SimpleIdServer.Scim.Domain.SCIMRepresentationExtensions.EnrichResponse(IEnumerable`1 attributes, JObject jObj, Boolean mergeExtensionAttributes, Boolean isGetRequest) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Extensions\SCIMRepresentationExtensions.cs:line 324
         at SimpleIdServer.Scim.Domain.SCIMRepresentationExtensions.ToResponse(SCIMRepresentation representation, String location, Boolean isGetRequest, Boolean includeStandardAttributes, Boolean addEmptyArray, Boolean mergeExtensionAttributes) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Extensions\SCIMRepresentationExtensions.cs:line 191
         at SimpleIdServer.Scim.Api.BaseApiController.InternalAdd(String prefix, RepresentationParameter jobj, CancellationToken cancellationToken) in C:\repos\work\cloned_repos\SimpleIdServer\src\Scim\SimpleIdServer.Scim\Api\BaseApiController.cs:line 359

In the exception message, notice on line 4 that they say "Can not add property groups to Newtonsoft.Json.Linq.JObject". That message was quite confusing at first because I thought "property groups" was a concept in JObject. But after googling and comparing my problem to other problems on Stackoverflow for example, this exception really means that there is a property called "groups" that gets added more than once to a JObject object. This led me to look in StandardSchemas in the Scim.Domain project, and there I noticed that "groups" was added twice to the static variable UserSchema.

A fix for this is to remove the second call to AddComplexAttribute("groups", ...) in StandardSchemas.cs (see the commits in this PR). I also made the subattributes in "groups" to be read-only, because they are read-only in the user schema in the "Schemas" directory of SimpeIdServer.Scim.Startup.

canea-asb commented 3 months ago

I don't know why AppVeyor build failed, the build succeeded locally. I don't think I can re-run the build either...

simpleidserver commented 2 months ago

Hello @canea-asb ,

First, thank you for your contribution to the project. The pull request has been accepted because it fixes both bugs.

I didn't notify them because the "SimpleIdServer.Scim.Startup" project does not use the StandardSchemas object. Instead, it reads the SCIM schemas from files, parses them, and inserts the results into the data storage.

You correctly identified some issues within this class. :)

PS : OpenID Federation is supported starting from version 5.0.1. You can follow this tutorial to implement a working scenario. :)

If you have any questions, feel free to contact me.

canea-asb commented 2 months ago

Great!

Nice that you support Open ID Federation. My question on gitter concerned the "FastFed" specification though. ☺️ Do you have any plans on implementing it?

simpleidserver commented 2 months ago

Oops, my bad! :)

We will start implementing fast-fed once we have completed the following tasks:

Update our mobile applications (iPhone and Android) to use the latest SimpleIdServer.WalletClient in order to present Verifiable Presentations to a credential issuer.
Publish the Android and iOS applications to their respective stores.
Submit our application process to comply with the EBSI conformance test for both "Request & Present VC" and "Issue VC" (https://hub.ebsi.eu/wallet-conformance).

simpleidserver commented 1 month ago

@canea-asb :

The release 5.0.2 has been published, and OPENID FastFed is now supported. 😊 To learn more about this topic, you can read this tutorial:

https://simpleidserver.com/docs/next/tutorial/fastfed

If you have any questions about how to implement it, I would be glad to help you.

simpleidserver / SimpleIdServer

Standard schema bugs #778

Do this before reproducing the bugs

Bug 1: Cannot specify the attribute "primary" in an address.

Bug 2: Crash when sending POST to /Users