search

pseymour / MakeMeAdmin

Make Me Admin is a simple, open-source application for Windows that allows standard user accounts to be elevated to administrator-level, on a temporary basis.
https://makemeadmin.com/
GNU General Public License v3.0
391 stars 85 forks source link

Merge JDM-feature-refactor-auth-check-send-identity #50

Closed jmoeller-ua closed 2 years ago

jmoeller-ua commented 2 years ago

Added the 3-parameter alternative to the UserIsAuthorized that sends the identity, since the 2-parameter version could idefnitify the wrong user on our test machines. Refactored some of the calling functions to refer to this new 3-param version.

Also added logging elements and a part of the LocalUI app to denote that its our forked version that is running.

jmoeller-ua commented 2 years ago

Heh, sorry about that. Meant to submit the PR to our forked version, not to the original. Misunderstood what clicking on that link Visual Studio would do!

pseymour commented 2 years ago

No problem, but it's interesting that you need to add the functionality that you did. The current user should never be System. Under what conditions does that happen?

jmoeller-ua commented 2 years ago

No problem, but it's interesting that you need to add the functionality that you did. The current user should never be System. Under what conditions does that happen?

I was running into this in the "Automatic Add Allowed" scenarios. In the MakeMeAdmin service, the SessionChangeReason.SessionLogon had the correct identity of the logged-in user, but, on my test system, the UserIsAuthorized function's ServiceSecurityContext.Current.WindowsIdentity was returning S-1-5-18. So I'm testing out an alternative to UserIsAuthorized that takes an identity (token), and I can alter callers to send the correct one along.

BTW, I'm certain this is some sort of misconfiguration or issue in my environment than being an issue with Make Me Admin. I thought perhaps initially that it was returning the identity of the account running the Windows Service, but I don't think that's right at all since the LocalUI/"Allowed Entities" was working well on my test system.

I did want to fork it and see if I could make a go of it on my test systems even given that I've likely got something extra going on it, since it's such a great piece of software and could solve a number of issues for us. I really appreciate you and Sinclair putting it out there, and I again apologize for being a dummy when it comes to git!

pseymour commented 2 years ago

Don't worry about the git submission. It's not a problem.

I was looking at the code before my last post, and I thought the automatic-add scenario might be where you were seeing the problem. It's good to have you confirm that. We use automatic-add for our technicians, so I will investigate this on my end also.

pseymour commented 2 years ago

Confirmed. This is a bug in the current repository. It was introduced sometime after version 2.3 was released, because that's the version we're on, and automatic adds work fine. Installing the current code in the repo breaks automatic additions.

pseymour commented 2 years ago

Problem should be fixed with the code I just pushed.

jmoeller-ua commented 2 years ago

Wonderful! I'll give this a shot on my test machines today. Thanks @pseymour!

jmoeller-ua commented 2 years ago

It works great on my test machine. I'll try it on some others as we continue our evaluation.

Well done!

There's some debug-type output in the Make Me Admin event log, including the "Validating" line on line 298 of LocalAdministratorGroup.cs, which fires with the timer every 10s. But I sort of line being able to see the rest of what's going on in the "Debug" version of the installer.

Thanks again, @pseymour!

pseymour commented 2 years ago

Yeah I'll take those out, but it should be compiled in Release mode for production anyway.