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

Feature/windows auth #559

Closed
wants to merge 5 commits into from
Closed

Feature/windows auth #559

wants to merge 5 commits into from

Conversation

xaviergxf
Copy link

@xaviergxf xaviergxf commented Mar 30, 2020

Hi,
I have revised and made some changes on the implementation:

  • Added support for multiple ad's
  • Background service for user sync
  • Use of domain full qualified name

Best regards

@xaviergxf
Copy link
Author

Hi @skoruba have you had time to look at this pr?

@skoruba
Copy link
Owner

skoruba commented Apr 18, 2020

Hi, sorry for delay, not yet. :-)

@skoruba skoruba changed the base branch from master to feature/windows-auth April 18, 2020 12:49
@skoruba skoruba changed the base branch from feature/windows-auth to dev April 18, 2020 12:49
@skoruba
Copy link
Owner

skoruba commented Apr 18, 2020

I would like to merge these changes into dev first, can you please check conflicts?
Thanks.

@sgebhardt
Copy link
Contributor

it would be awesome if this get merged. Can i help with this? I use xaviergxf branch on my develop system and it works like intended.

@xaviergxf
Copy link
Author

i have fixed again de merge conflicts

@skoruba
Copy link
Owner

skoruba commented May 18, 2020

Sorry guys for delay, currently I am busy - I will check this PR later, because this PR is pretty complex and I need test it and check all details. ;)

@xaviergxf
Copy link
Author

thanks @skoruba

@sgebhardt
Copy link
Contributor

thanks @skoruba
Maybe wie should merge this first into the windows-auth branch. The branch is mentioned in #479. So it is easier for others like @thomas-illiet to see the progress on this.

@vpetkovic
Copy link

FYI - I had no issues implementing @xaviergxf solution into master branch - works like a charm

@skoruba
Copy link
Owner

skoruba commented Jun 15, 2020

@vpetkovic - thanks for feedback 👍

@xaviergxf
Copy link
Author

@skoruba, any idea when will this be merged? Next release ?

@skoruba skoruba changed the base branch from dev to feature/windows-auth September 22, 2020 19:15
@skoruba skoruba changed the base branch from feature/windows-auth to dev September 22, 2020 19:16
@skoruba skoruba changed the base branch from dev to feature/windows-auth-v2 September 24, 2020 19:17
@skoruba
Copy link
Owner

skoruba commented Sep 24, 2020

I've prepared fresh branch called feature/windows-auth-v2. This branch contains changes from master and everything from original feature/windows-auth. I will check your changes which you made also.
Thanks for your work on this feature. 👍

@sgebhardt
Copy link
Contributor

I have tested the new branch. Login and user handling works fine for me.

But a can not figure out why the "Import All Windows User" buton is not visible. It looks like there is no change on the buton logic at all. Is there a change with the external logins?

@sgebhardt
Copy link
Contributor

sgebhardt commented Oct 6, 2020

But a can not figure out why the "Import All Windows User" buton is not visible. It looks like there is no change on the buton logic at all. Is there a change with the external logins?

I found that the "IISServerAuthenticationHandler" was not registered at ASP.NET Core Identity.
I had to add the IISServerOptions additionally to the IISOptions to the ConfigureServices() method in Startup.cs

services.Configure<IISServerOptions>(iis =>
{
     iis.AuthenticationDisplayName = "Windows";
     iis.AutomaticAuthentication = false;
});

I created a pull request to fix this. #726

@willisga
Copy link

willisga commented Oct 13, 2020

Hello @skoruba

Thank you for such excellent work and to all the developers who accompany you.

I was validating the conversations and I wanted to know by what date the authentication windows will be released, which I understand is in the features/windows-auth-v2 branch.

I also wanted to know if this features/windows-auth-v2 branch is finished to start testing in our development team

Thank you very much and I will be pending.

@vpetkovic
Copy link

With the newest releases of chrome/edge Version 86.0.622.56 (Official build) (64-bit) I started experiencing issues with GetExternalLoginInfoAsync() method on line 469 as it is constantly returning null.

Not sure if any of you ran into that with new versions of chromium browser? I might be missing out on something 🤔. Firefox for instance works with no issue.

@skoruba
Copy link
Owner

skoruba commented Oct 28, 2020

@aiscrim - any idea? 😊

@aiscrim
Copy link
Contributor

aiscrim commented Oct 28, 2020

@vpetkovic I don't know, there are several possible reasons why that method can returns null. Try to substitute the call to the _signinManager method with the following and let me know in which of the two "return null" you get caught. Also, while you debug it check if the "Identity.External" cookie is present in your browser or if there are any problems with it.

public virtual async Task<ExternalLoginInfo> GetExternalLoginInfoAsync()
        {
            string LoginProviderKey = "LoginProvider";
            var auth = await this.HttpContext.AuthenticateAsync(IdentityConstants.ExternalScheme);
            var items = auth?.Properties?.Items;
            if (auth?.Principal == null || items == null || !items.ContainsKey(LoginProviderKey))
            {
                return null;
            }

            var providerKey = auth.Principal.FindFirstValue(ClaimTypes.NameIdentifier);
            var provider = items[LoginProviderKey] as string;
            if (providerKey == null || provider == null)
            {
                return null;
            }

            var providerDisplayName = "Windows";
            return new ExternalLoginInfo(auth.Principal, provider, providerKey, providerDisplayName)
            {
                AuthenticationTokens = auth.Properties.GetTokens()
            };
        }

@vpetkovic
Copy link

@skoruba & @aiscrim thanks for quick response - I have tested what you've suggested and the first one returned null. However, in neither Firefox or chrome/edge I have Identity.External set. Only the following in FF.:

image

image

So, neither of you could replicate it in chrome/edge (assuming that's your browser of choice)?

@aiscrim
Copy link
Contributor

aiscrim commented Oct 29, 2020

@vpetkovic I am using Chrome 86.0.4240.111 and Edge 86.0.622.38, so slightly different versions than what you have, but I don't have any problems.
If you don't have that cookie set you will always receive null there. It's set here, so something must be going on there. Maybe Chromium for some reason is blocking that cookie?
Question is: why in Firefox you don't see it and it works? Probably you're looking at it not while debugging. Beware that it's a volatile cookie, that's set upon call to IssueExternalCookie and then removed upon successfull login, so if you don't put a breakpoint in the ExternalLoginCallback method you'll never see it.

@vpetkovic
Copy link

Something is definitely up and I bet it's something ridiculous.

You are right - so I didn't pay attention on that specific breakpoint but now that I do, only Firefox is grabbing it. Is there anything else that comes to your mind, why it's not able to set them up that I should check?

I have tested on one other VM where I had older version (I think 85) and had no issues, but as soon as it updated to 86 it started happening. So it is wide spread, at least in our large organization. Just the wild thought, but we have, still hard to get rid of, old controller (win 2003) that works alongside 2016 on the same domain, and wondering if it has anything to do with it, even a bit, since you are saying it works fine with you even on version 86.

Only Firefox
image

Both Chromium and FireFox
image

@aiscrim
Copy link
Contributor

aiscrim commented Oct 29, 2020

@vpetkovic You may try to put a breakpoint here and see what happens, just to see if it's at least trying to set the cookie or not.
Also, I suppose your Identity Server is served through HTTPS, right? Because if it was via HTTP Chrome could be rejecting the cookie...
I have no other ideas sorry, I hope this above will help you a bit.

@vpetkovic
Copy link

Thank you @aiscrim for all the ideas and quick responses! It turned out to be a time consuming debugging and without wasting further of your or anyone's time, I decided to clone the branch again, reconfigure it to our needs and test it out. So far so good. Version of Chromium wasn't an issue anymore.

It must've been something weird with the branch that I pulled back in June that become an issue with most recent chromium releases as an issue was easily reproduced

@ruzanowski
Copy link

Great job guys, I intend to test it out too, so will let you know the feedback in days.

However, it needs some work to resolve conflicts.

Thanks!

@skoruba
Copy link
Owner

skoruba commented Nov 26, 2020

Hey guys, I have to merge release/2.0.0-beta1 with this branch, because it is not compatible with IS4 version 4. 😊

@gordnian
Copy link

gordnian commented Jul 6, 2021

Hi,
Is there any plan to merge this branch to master?

@xaviergxf xaviergxf closed this by deleting the head repository Jun 5, 2023
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.

8 participants