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

Handle systems with multiple users #24

Conversation

GurliGebis
Copy link
Contributor

@GurliGebis GurliGebis commented Apr 5, 2023

This handles multi user systems.
Since we cannot install a sparse package (msix) for all users on the system (due to the way the store and it's services are architected), we have to install it specifically for every user.

We do this by using the fact that the DLL is registered in the registry, so when the user right clicks a file for the first time, the DLL is loaded into explorer.exe's address space.
So we do the following:

  • Ensure the process we are being loaded into is "explorer.exe", since we really don't want to do anything like this if we are being loaded into "regsvr32.exe"
  • Once we know the process is the right one, we spawn a new thread to avoid freezing the UI while we run.
  • In that thread, we start by getting the package - if it exists already, we stop, since nothing has to be done.
  • If the package is missing, we register it and notifies the UI that we have made changes.

The cool thing about this, is that we only run the check when the user right click for the first time in their session, since that is the only time the DLL is loaded into explorer.exe - after that, it stays in memory for the rest of their session.

The only downside is that the very first time a user (that isn't the one who installed Notepad++) right clicks a file, the menu items will be missing, since we haven't had a check to register the items yet. (Since our code isn't running before then, we cannot do it before this).

This fixes notepad-plus-plus/notepad-plus-plus#13476

@donho
Copy link
Member

donho commented Apr 5, 2023

@GurliGebis

NppShell.dll is supposed to provide "Edit with Notepad++" context menu, but now it's getting more and more complex.

I do understand that you're trying to fix all the issues that we've discovered, but I prefer to wait and to see users' feedback.

Obviously RC publication doesn't help and we need more users feedback for what you have implemented.

This PR will be merged, if it's really necessary (ie. a lot of users have the same problem and this solution is tested by them).

Hope you understand my concern.

@GurliGebis
Copy link
Contributor Author

@donho sure, I understand 🙂
Everyone with a PC with more than one user (like in a family), will be needing this code, since if it isn't there, only the one user who did the initial installation has the context menu elements. (Due to the way the store services works unfortunately)
So I think we should include it - but maybe wait till after 8.5.2, and then do a test build for people in that issue to test with - what do you think about that idea?

@GurliGebis GurliGebis force-pushed the handle-multiuser-systems branch from 0bfabe2 to 99461b6 Compare April 6, 2023 14:59
@GurliGebis GurliGebis force-pushed the handle-multiuser-systems branch from 99461b6 to e512237 Compare April 7, 2023 09:14
@donho
Copy link
Member

donho commented Apr 8, 2023

@GurliGebis
Just checked this PR, it doesn't work with the local test account (login: test user, neither ms email account nor tel number).

@GurliGebis
Copy link
Contributor Author

@donho I'll push a branch with some logging later today or tomorrow, if you can try and build that locally and run it, and give me the log file, that would be great.
I'll let you know once it is ready. (Got a few things I need to take care of now)

@GurliGebis
Copy link
Contributor Author

@donho Here you go: https://github.com/GurliGebis/nppShell/tree/multi-user-debug
I have uploaded a video showing how I tested it here: https://youtu.be/Bkhvs9lYEXo

Here is how to test it:

  1. Install the normal 8.5.2 on a user with admin rights
  2. Create a new user on the machine (doesn't need to have admin rights).
  3. Login as the user, and verify that the context menu is missing for the user.
  4. Log back into the user who installed Notepad++
  5. Build the branch I linked above, sign the DLL and replace the one in the contextMenu folder with it.
  6. Log back into the new user.
  7. Right click a file - the menu will be empty (first time).
  8. Wait a few seconds, right click the file again - now it has been installed.

Please look inside the %APPDATA%\NppShell folder for the NppShell.txt file, please send me the content of that file if it doesn't work.

@donho donho removed the suspended label Apr 9, 2023
@donho donho self-assigned this Apr 9, 2023
@donho
Copy link
Member

donho commented Apr 9, 2023

@GurliGebis
It didn't work after I installing TEST15 which contains this PR.
It does work now - I think my Win11 understand how to work after watching your video :)
(Joke aside, I certainly right clicked only on folder shortcut of Quick Access)

It looks promising. Here are the packages of TEST15:
http://download.notepad-plus-plus.org/repository/MISC/nppShell.TEST15/

@donho
Copy link
Member

donho commented Apr 9, 2023

@GurliGebis

The cool thing about this, is that we only run the check when the user right click for the first time in their session, since that is the only time the DLL is loaded into explorer.exe - after that, it stays in memory for the rest of their session.

Tested - the menu entry is missing only on the very first time. So it's OK I think.

@GurliGebis
Copy link
Contributor Author

GurliGebis commented Apr 10, 2023

@donho it works for me as well 🙂
Just tested the TEST15 installer on a clean machine with an admin user and a normal user.

@donho donho added the accepted Extra attention is needed label Apr 10, 2023
@donho donho closed this in 059feff Apr 10, 2023
@GurliGebis GurliGebis deleted the handle-multiuser-systems branch April 11, 2023 07:05
@ddomingos-encora
Copy link

@GurliGebis and @donho Since the msix package needs to be registered per user, I think you guys had a great solution with the method EnsureRegistrationOnCurrentUser. However, it has a blind spot: the uninstallation. The uninstaller will use regsvr32 /u which will uninstall only for the user that is executing the uninstaller, leaving the package still installed for other users. I saw that winmerge uses some bat scripts that call powershell commands directly to register/unregister but I could not find if they are calling to all users in the machine.

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora There really is no clean way to remove it from all users on the system, but since the msix is gone, it shouldn't cause any issues.

@ddomingos-encora
Copy link

ddomingos-encora commented Nov 18, 2023

Hi @GurliGebis
I found a solution for this issue. It's possible to install/uninstall the package for all users. You need admin privileges to call these APIs but I think the installer already asks for it. To achieve this, you need only 3 changes to the code in Installer.cpp:

  1. Replace the FindPackagesForUser to FindPackages
    packages = packageManager.FindPackages();

  2. Instead of using AddPackageByUriAsync, stage the package and then provision it to all users. Staging API is very similar to adding API and, according to documentation, provisioning a package install it for other users when they sign in the next time after the package is provisioned. For the current user it is installed right away. The modified code is:

    PackageManager packageManager;
    StagePackageOptions options;

    const wstring externalLocation = GetContextMenuPath();
    const wstring sparsePkgPath = externalLocation + L"\\NppShell.msix";

    Uri externalUri(externalLocation);
    Uri packageUri(sparsePkgPath);

    options.ExternalLocationUri(externalUri);

    auto deploymentOperation = packageManager.StagePackageByUriAsync(packageUri, options);
    auto deployResult = deploymentOperation.get();

    if (!SUCCEEDED(deployResult.ExtendedErrorCode()))
    {
        return deployResult.ExtendedErrorCode();
    }

    Package package = GetSparsePackage();
    if (package == NULL)
    {
        return S_FALSE;
    }

    winrt::hstring familyName = package.Id().FamilyName();

    deploymentOperation = packageManager.ProvisionPackageForAllUsersAsync(familyName);
    deployResult = deploymentOperation.get();

    if (!SUCCEEDED(deployResult.ExtendedErrorCode()))
    {
        return deployResult.ExtendedErrorCode();
    }

    return S_OK;
  1. Pass a flag when removing the package, so it is removed for all users:
    auto deploymentOperation = packageManager.RemovePackageAsync(fullName, RemovalOptions::RemoveForAllUsers);

If you think it's a good approach, I can create a PR. Regards.

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora I can take a look at it.
I did try something similar earlier, and the problem I ran into was that it only got stage, but never actually installed for users.

@GurliGebis
Copy link
Contributor Author

GurliGebis commented Nov 18, 2023

@ddomingos-encora come to think of it.
If you can create a PR, but against my repo (easier for me to test, due to my git setup being strange), that would be great 🙂

Url: https://github.com/GurliGebis/nppShell

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora I tried it myself.
The package is listed if I run Get-AppxPackage -AllUsers , but it never gets installed for the users.

This is the original problem I ran into - when provisioning a sparse package, it just never gets installed for users, it just stays in the AllUsers list.

Did you test it locally, to verify that it actually got installed?

@GurliGebis
Copy link
Contributor Author

All users:

PS C:\Windows\system32> Get-AppxPackage -AllUsers *notepad* |ft

Name                     Publisher                                                                        PublisherId   Architecture ResourceId Version      PackageFam
                                                                                                                                                             ilyName
----                     ---------                                                                        -----------   ------------ ---------- -------      ----------
Microsoft.WindowsNotepad CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US 8wekyb3d8bbwe          X64            11.2112.32.0 Microso...
NotepadPlusPlus          CN="Notepad++", O="Notepad++", L=Saint Cloud, S=Ile-de-France, C=FR              7njy0v32s6xk6      Neutral            1.0.0.0      Notepad...

Current user:


PS C:\Windows\system32> Get-AppxPackage *notepad* |ft

Name                     Publisher                                                                        PublisherId   Architecture ResourceId Version      PackageFam
                                                                                                                                                             ilyName
----                     ---------                                                                        -----------   ------------ ---------- -------      ----------
Microsoft.WindowsNotepad CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US 8wekyb3d8bbwe          X64            11.2112.32.0 Microso...

@GurliGebis
Copy link
Contributor Author

Also, by looking at the documentation, it looks like provisioning packages is for when creating a wim installation file.
So provisioning packages after Windows has been installed won't do anything (unless I'm reading it wrong)

@ddomingos-encora
Copy link

ddomingos-encora commented Nov 20, 2023

Hi @GurliGebis
I tested and it worked for me. I have one suspicion why it might not be working for you: maybe when you updated the code and changed the function FindPackagesForUser to FindPackages, you kept the empty string parameter (it should be no parameters now). If that's the case, it won't find the package and the final result will be that the package will be staged but not provisioned. It's important to notice that since the registration code happens in another thread, you can't get the error and regsvr32 will always succeed.
If it still fails, we can try to add some debug code. Not sure how you do it, but I use the function OutputDebugString and use the Debug View (https://learn.microsoft.com/en-us/sysinternals/downloads/debugview) to view these logs. In this case, you could print deploymentResult.ErrorText()
Let me know how it goes.

p.s: I'd like to ask you some help. I ended up messing my Windows. I generated the DLL and MSIX and to not have to use long paths I copied them to C: The problem is that when unregistering, the code calls a function related to ACL and this completely changed the permissions of my C: What does that function do? Do you know how could I revert its effect to get access again to my C: drive? Thanks

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora you are right - I will test again later today (only had 4 hours of sleep last night)

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora it does it, to prevent issues with the taskbar icon, since if the dll and msix is located in the same folder as the main exe, things break.
That is why it is installed in a subfolder, and the ACL permissions are set on that instead.

There is no backup of the ACL's, so if you C drive is all messed up by having them in the root - I don't think there is an easy way to correct it (maybe do an upgrade to the same Windows version using the ISO - that might fix it - just a thought though, not something I have tested)

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora okay, it seems like with using an empty string, it gets installed.
However, with the package installed, it doesn't seem to work. (The Get-AppxPackage cmdlet shows it as installed, but it isn't registered).

I have pushed my changes here - can you take a look and see if you can spot anything wrong?
It is here: #55

What I have done is the following:

  1. Clean Windows install with two users created.
  2. Installed latest Notepad++ on user 1.
  3. Unregister the dll with regsvr32 /u NppShell.dll
  4. Build and sign the code with a trusted certificate.
  5. Copy the newly build and signed dll and msix into the contextMenu folder, overwriting the two existing files.
  6. Register the dll with regsvr32 NppShell.dll
  7. It doesn't work on either user 1 or user 2, but both have it installed.

@ddomingos-encora
Copy link

Hi @GurliGebis
I'll take a look but I will need to recover my system first. At least I managed to save my files. It seems the problem was only in C: (not recursively) or I managed to restore access to subfolders using icacls command.
When I tested, I saw that the package got installed but when I right-clicked a file the menu indeed did not show. I could not debug because of the issue I got but my guess is that there is some code failing and this results in the menu entry not displayed. For example, since my package was on C: it would not find the main Notepad++ executable. So I would test like you did, reconstructing the same structure as it would be if installed by the installer. I'll let you know once I'm able to test

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora thanks.
Lets move the discussion into the new draft PR 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Extra attention is needed
Projects
None yet
3 participants