notepad-plus-plus / nppShell

Provide Explorer context menu entry "Edit with Notepad++"
GNU General Public License v3.0
25 stars 13 forks source link

Handle systems with multiple users #24

Closed GurliGebis closed 1 year ago

GurliGebis commented 1 year ago

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:

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 commented 1 year ago

@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 commented 1 year ago

@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?

donho commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

ddomingos-encora commented 1 year ago

@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 commented 1 year ago

@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 commented 12 months ago

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;

3) 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 commented 12 months ago

@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 commented 12 months ago

@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 commented 12 months ago

@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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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

GurliGebis commented 12 months ago

@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 commented 12 months ago

@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: https://github.com/notepad-plus-plus/nppShell/pull/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 commented 12 months ago

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 commented 12 months ago

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