davidjrh / dnn.azureadprovider

The DNN Azure Active Directory Provider is an Authentication provider for DNN Platform (formerly DotNetNuke) that uses Azure Active Directory OAuth2 authentication to authenticate users.
MIT License
35 stars 22 forks source link

User can't be create if another has same nickname #53

Closed rodsmr closed 2 years ago

rodsmr commented 2 years ago

Hi @davidjrh , I've got this problem: can you help me to find a way to solve it? In my site, with private registration, I've two kind of login: one with your module, one with SPID (Public Digital Identity System (SPID) is the simple, fast and secure access key to digital services of local and central administrations)

The login phase goes when

  1. User login with SPID: it doesn't exist and it's created on portal DB
  2. Same user login with AAD: it doesn't exist, the process goes in error. From Admin Log, I've got this error The logged in user azure-MYEMAIL does not belong to PortalId 0

This is the inner stack trace

at DotNetNuke.Authentication.Azure.Components.AzureClient.AuthenticateUser(UserData user, PortalSettings settings, String IPAddress, Action`1 addCustomProperties, Action`1 onAuthenticated)
   at DotNetNuke.Services.Authentication.OAuth.OAuthLoginBase.OnLoad(EventArgs e)
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.LoadRecursive()
   at System.Web.UI.Control.<LoadRecursiveAsync>d__246.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Util.WithinCancellableCallbackTaskAwaitable.WithinCancellableCallbackTaskAwaiter.GetResult()
   at System.Web.UI.Page.<ProcessRequestMainAsync>d__523.MoveNext()

I think this error is caused by DisplayName field: SPID and AAD user have the same value

Thanks for the help

davidjrh commented 2 years ago

I don't think the DisplayName is causing this problem, I've tested other scenarios having mix of Auth providers working well. Can you share more details?

rodsmr commented 2 years ago

Hi @davidjrh , thanks for the response Question 1: DNN instance has 1 portal Question 2: there isn't deleted users

From better code analysis, I found AuthenticateUser method in Components/AzureClient.cs class. I put my eyes on this part of method

var usernamePrefixEnabled = bool.Parse(AzureConfig.GetSetting(AzureConfig.ServiceName, "UsernamePrefixEnabled", portalSettings.PortalId, "true"));
var usernameToFind = usernamePrefixEnabled ? $"azure-{userClaim.Value}" : userClaim.Value;
var userInfo = UserController.GetUserByName(portalSettings.PortalId, usernameToFind);
// If user doesn't exist on current portal, AuthenticateUser() will create it. 
// Otherwise, AuthenticateUser will perform a Response.Redirect, so we have to sinchronize the roles before that, to avoid the ThreadAbortException caused by the Response.Redirect
if (userInfo == null)
{
    base.AuthenticateUser(user, portalSettings, IPAddress, addCustomProperties, onAuthenticated);
    if (IsCurrentUserAuthorized())
    {
        userInfo = UserController.GetUserByName(portalSettings.PortalId, usernameToFind);
        if (userInfo == null)
        {
            throw new SecurityTokenException($"The logged in user {usernameToFind} does not belong to PortalId {portalSettings.PortalId}");
        }
        UpdateUserAndRoles(userInfo);
        MarkUserAsAad(userInfo);
    }
}
else
{
    if (IsCurrentUserAuthorized())
    {
        UpdateUserAndRoles(userInfo);
    }
    base.AuthenticateUser(user, portalSettings, IPAddress, addCustomProperties, onAuthenticated);
}

There is this comment

// If user doesn't exist on current portal, AuthenticateUser() will create it. // Otherwise, AuthenticateUser will perform a Response.Redirect, so we have to sinchronize the roles before that, to avoid the ThreadAbortException caused by the Response.Redirect

In my case, userInfo IS NULL so it calls base.AuthenticateUser...but this base method doesn't create the user and it raise the exception The logged in user {usernameToFind} does not belong to PortalId {portalSettings.PortalId}

If you need more info, tell me

Thanks, rodsmr

davidjrh commented 2 years ago

Can you share the collation of your database? I'm wondering if this line is causing the issue because the prefix should be "Azure-" instead of "azure-" being case sensitive

var usernameToFind = usernamePrefixEnabled ? $"azure-{userClaim.Value}" : userClaim.Value;

Anyway, I'm going to fix this to don't fail on case sensitive databases var usernameToFind = usernamePrefixEnabled ? $"{AzureConfig.ServiceName}-{userClaim.Value}" : userClaim.Value;

rodsmr commented 2 years ago

Collation is SQL_Latin1_General_CP1_CI_AS. Table Users, Username field, has this value: Azure-MYEMAIL

I try your changes but it doesn't work: same error :(

davidjrh commented 2 years ago

And in the Users table, is the user also with the correct PortalId? I'm wondering how this line of code is not returning a user. Can you check the value of portalSettings.PortalId and if there is any whitespace on the claim or on the Username field in the database?

userInfo = UserController.GetUserByName(portalSettings.PortalId, usernameToFind);

rodsmr commented 2 years ago

Hi @davidjrh , I've found the problem: DNN doesn't accept two users with same DisplayName. Look the attached image, from Users admin panel image

In the code, I add a try/catch block

// If user doesn't exist on current portal, AuthenticateUser() will create it. 
// Otherwise, AuthenticateUser will perform a Response.Redirect, so we have to sinchronize the roles before that, to avoid the ThreadAbortException caused by the Response.Redirect
if (userInfo == null)
{
    TRY
    {
        base.AuthenticateUser(user, portalSettings, IPAddress, addCustomProperties, onAuthenticated);
        Logger.Error(String.Format("user var: {0}", Newtonsoft.Json.JsonConvert.SerializeObject(user)));

        if (IsCurrentUserAuthorized())
        {
            Logger.Error(String.Format("portalSettings.PortalId #2 var: {0}", Newtonsoft.Json.JsonConvert.SerializeObject(portalSettings.PortalId)));
            Logger.Error(String.Format("usernameToFind #2 var: {0}", Newtonsoft.Json.JsonConvert.SerializeObject(usernameToFind)));
            userInfo = UserController.GetUserByName(portalSettings.PortalId, usernameToFind);
            Logger.Error(String.Format("userInfo #2 var: {0}", Newtonsoft.Json.JsonConvert.SerializeObject(userInfo)));
            if (userInfo == null)
            {
                throw new SecurityTokenException($"The logged in user {usernameToFind} does not belong to PortalId {portalSettings.PortalId}");
            }
            UpdateUserAndRoles(userInfo);
            MarkUserAsAad(userInfo);
        }
    }
    CATCH(Exception e)
    {
        Logger.Error(String.Format("base.AuthenticateUser not work: {0}", e.Message));
    }
}
else
{
    if (IsCurrentUserAuthorized())
    {
        UpdateUserAndRoles(userInfo);
    }
    base.AuthenticateUser(user, portalSettings, IPAddress, addCustomProperties, onAuthenticated);
}

and now I can see the message "The Display Name is already in use."

Thanks for the support

PS: I report my log message

user var: {"given_name":"MYFIRSTNAME","family_name":"MYSURNAME","name":"MYFIRSTNAME MYSURNAME","id":"MYEMAIL","email":"MYEMAIL","emails":null,"gender":null,"locale":null,"timezone":null,"time_zone":null,"username":null,"website":null} portalSettings.PortalId #2 var: 0 usernameToFind #2 var: "Azure-MYEMAIL" userInfo #2 var: null base.AuthenticateUser not work: The logged in user Azure-MYEMAIL does not belong to PortalId 0

davidjrh commented 2 years ago

@rodsmr by default, DNN supports duplicated display names unless you go and change that setting on Security > Member Accounts > Registration Settings. I will try to accommodate the code for that setting failing gracefully.

image

rodsmr commented 2 years ago

@davidjrh ah ok, in my case Require Unique Dispaly Name is set to true

rodsmr commented 2 years ago

Hi @davidjrh , I've got another "problem": sometimes, when AAD login create a new user firstname and lastname are inverted. I've not access to Azure Active Directory but the manteiner tells me the data are saved correctly

How can I indagate?

davidjrh commented 2 years ago

The easiest way would be to inspect the token that is stored in the browser cookie and inspect the claims. You can use the "EditThisCookie" Chrome/Edge addon, and then paste the content of the cookie on https://jwt.io to decode the token.

Other approach would be, if you have setup advanced settings with the App/Secret for "Application" type permissions, to query directly the Graph API to check those users.