Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User Access module implementation with the microsoft identity framework. #295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jfstirn
Copy link

@jfstirn jfstirn commented Dec 19, 2023

User Access module based on Microsoft Identity Framework.

I just added the whole module without removing the existing one. Just that you can check if it meets your expectations.

Futhermore I didn't know how you wolud structrue the hole project to have 3 implementations of the user access module.

Can you please check and let me know how I or we should proceed with this?

Best regards

@kgrzybek
Copy link
Owner

Hi @jfstirn! It is a huge PR, I know what I will be doing during Christmas ;)

I will check it - give me some time to do that :)

CC: @bistok - please check it because you wanted to do it yourself

@bistok
Copy link
Contributor

bistok commented Dec 19, 2023

@kgrzybek I have checked a bit, I downloaded the PR code, and currently they fail the Integration test (There is no test to the new functionality).

I do not like the Idea that every class in the UserModule projects is duplicated and only a small quantity are new clases, I think there is maybe a better approach to do this (thinking how to do it). Almost in 400 files the unique change is the using directive.

Because on the future if we would like to add something to the User Module we need to change in 2 places the same and that is not maintainable, even if we do a third user module implementation, that will be a nightmare to maintain. But if we will only replace everything it's OK, but if we like to use back Identity Server with the changes, need to change all the files??

And there will not be an easy option to Swap the implementations on runtime, only on compile time.

I like:

  • the Idea for the Contracts Project and the IEntity Interface.
  • The database project has all the new tables and the seeds

@jfstirn
Copy link
Author

jfstirn commented Dec 20, 2023

@kgrzybek, @bistok Thank you both for your time.

Almost in 400 files the unique change is the using directive.

That's because I renamed the original project. I'm sorry about that.

I think the new implementation can be seen as the main User Module and we don't need more than one. It can be used independent or in combination with any Id-Provider.

If you would like to add more Id-Providers, than they should be implemented as separate applications. Normally you would have one Id-Provider serving multiple applications and not one per project.

Just like all Id-Providers they must be configured in the API project like so.

services
    .AddAuthentication()
    .AddMicrosoftIdentityWebApi(options =>
    {
        Configuration.Bind("AzureAdB2C", options);
        options.TokenValidationParameters.NameClaimType = "name";
    },
    options => { Configuration.Bind("AzureAdB2C", options); }, "AzureAd");


services.Configure<JwtBearerOptions>("AzureAd", options =>
{
    var existingOnChallenge = options.Events.OnChallenge;
    options.Events.OnChallenge = async context =>
    {
        await existingOnChallenge(context);

        context.Response.ContentType = "application/json";
        context.Response.StatusCode = StatusCodes.Status401Unauthorized;
        context.Response.OnStarting(() =>
        {
            var error = Errors.Authentication.NotAuthorized();
            string result = JsonSerializer.Serialize(Result.Error(error.ToErrorMessages()));

            return Task.CompletedTask;
        });
    };

Here comes the interesting part where we call into the User Access module

    var existingOnTokenValidatedHandler = options.Events.OnTokenValidated;
    options.Events.OnTokenValidated = async context =>
    {
        await existingOnTokenValidatedHandler(context);

        // Well the token has been validated, now we need to check if the user has an account

        // We really need the external user identifier
        var externalUserId = context.Principal.FindFirstValue("sub")
            ?? context.Principal.FindFirstValue(ClaimTypes.NameIdentifier)
            ?? throw new Exception("Cannot find external user id");

        // Get the provider from the scheme item
        var provider = context.Scheme.Name;

      // In this case we expect that the email addess is the relation between the Id-Provider and our users in the user access module, but there could also be a custom defined claim on both sides
        var identity = context.Principal.Identity as ClaimsIdentity;
        var emailAddress = identity.Claims.FirstOrDefault(x => x.Type == "email")?.Value
            ?? identity.Claims.FirstOrDefault(x => x.Type == ClaimTypes.Email)?.Value
            ?? identity.Claims.FirstOrDefault(x => x.Type == "emails")?.Value ?? "";

        var userAccessModule = context.HttpContext.RequestServices.GetService<IUserAccessModule>()!;

        // Once we have all this we can go ahead an call the external login command
        var result = await userAccessModule.Send(new ExternalAccountLoginCommand(provider, externalUserId, emailAddress, false));

        if (!(result?.IsAuthenticated ?? false))
            context.Fail(Errors.UserAccess.InvalidUserNameOrPassword.Code);
        // else 
        // Authentication succeeded:
        // May be update the user / permissions based on the claims form the bearer token?
    };
});

@bistok
Copy link
Contributor

bistok commented Dec 20, 2023

@jfstirn I am with you if we only will keep Microsoft Identity Framework that is OK and we can keep with you implementation, but need to update the test projects.

If we like to have multiple implementations, we need to follow other process. That is something that @kgrzybek can say.

You can see the conversation we have on #289

@kgrzybek
Copy link
Owner

@jfstirn, @bistok

I have analyzed the entire solution in terms of various user handling implementations.

  1. The current architecture does not support this, as the UserAccess module currently combines two modules - access and registration. Registration is specific, while access is generic. To be able to use different implementations, these modules need to be separated, especially since registrations publish events, which does not occur for generic domains/modules. They only expose APIs and possibly simulate event-driven behavior through web hooks.

  2. An additional problem is at the API level. Currently, there was no need to place the API layer in individual modules, but in the case of wanting to choose the implementation, the Users module should also contain the API.

  3. Regarding this pull request - it will definitely be useful, but a architectural change from point 1 is needed first, and only then can it be implemented. I will try to make this change as soon as possible and let you know.

  4. As for the implementation itself - I haven't delved deeply into the details yet, but certainly the lack of automated tests is a blocker because there is a lot of logic involved. Without proper tests, it's harder to understand, and we won't be able to refactor it later.

I am attaching the current state (as-is) and the desired state (to-be).
Instant - Frame 1

Instant - Frame 2

@kgrzybek kgrzybek mentioned this pull request Dec 30, 2023
@bistok
Copy link
Contributor

bistok commented Dec 31, 2023

I like the proposed implementation, with it we decuple the authentication implementation from the user registration.

  1. The current architecture does not support this, as the UserAccess module currently combines two modules - access and registration. Registration is specific, while access is generic. To be able to use different implementations, these modules need to be separated, especially since registrations publish events, which does not occur for generic domains/modules. They only expose APIs and possibly simulate event-driven behavior through web hooks.

This is true and that was the reason for duplication.

  1. An additional problem is at the API level. Currently, there was no need to place the API layer in individual modules, but in the case of wanting to choose the implementation, the Users module should also contain the API.

This will convert the solution from a Modular Monolith to a micro services.

  1. Regarding this pull request - it will definitely be useful, but a architectural change from point 1 is needed first, and only then can it be implemented. I will try to make this change as soon as possible and let you know.

Ok, Good to know, lets see what happen from this ;)

  1. As for the implementation itself - I haven't delved deeply into the details yet, but certainly the lack of automated tests is a blocker because there is a lot of logic involved. Without proper tests, it's harder to understand, and we won't be able to refactor it later.

I was refactoring the test code for it to run, but will stop that work waiting for number 3.

@carlsixsmith-moj
Copy link

It's difficult to demonstrate three separate ways of implementing identity without it becoming totally confusing.

I'm going to look into replacing IDS4 with the new Microsoft identity. We don't really need everything IDS4/Duande gives you, we will only have one client (a blazor front end)

@achmstein
Copy link

Is there any news regarding using Microsoft Identity instead of IdentityServer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants