fix(windows): set basekeyboard as current user not the admin user on an elevated process

[rc-swag]

Fixes: #15152

For all the iterartions this change went through you can read through the rest of this PR.

This PR now shows the BaseKeyboard form without elevation for the current user. On successful change it checks to see if there are already compiled keyboards for the selected basekeyboard. If there aren't compiled keyboards it then elevates to compile the keyboards but passes in the current user's basekeyboard ID. On successful return from the elevated process it sets the basekeyboard as selected by the user.

For installing keyboards it is also important that if the basekeyboard is different the admin user used for elevation that keyboards are compiled against the correct basekeyboard. This change is made in #16528

Build-bot: release:windows

User Testing

TEST_BASE_KEYBOARD_CURRENT_USER

1. Login into Windows with and account that is a "standard" user and does not have "Administrator" rights.

2. Install the Keyman from this PR

3. Open Keyman Configuration -> Keyboard Layouts

4. Install a keyboard for example sil_ipa

5. Open Keyman Configuration -> Options

Check C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa don't delete the base sil_ipa.kmx but delete any with KLIDs for example sil_ipa-00000409-d.kmx

6. Click Base Keyboard, change the Base Keyboard to German. You will need to enter the login details for a Admin user.

7. Confirm the Keyboard changes for the current user and not the Admin user used for the elevated processs.

8. Check C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa there should now be a sil_ipa-????0407-d.kmx and sil_ipa-????0407.kmx

TEST_BASE_KEYBOARD_CURRENT_USER_KMX_EXISTS

After completing the steps in TEST_BASE_KEYBOARD_CURRENT_USER

1. Open Keyman Configuration -> Options
2. Click Base Keyboard, change the Base Keyboard to English. You may need to enter the login details for a Admin user.
3. Close Configuration
   Check German mcomplied kmx is still there i.e. C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa there should now be a sil_ipa-????0407-d.kmx and sil_ipa-????0407.kmx
4. Open Keyman Configuration -> Options
   6.Click Base Keyboard, change the Base Keyboard to German. You should Not be asked to enter a admin user.

[keymanapp-test-bot]

User Test Results

Test specification and instructions

• ✅ TEST_BASE_KEYBOARD_CURRENT_USER (PASSED) (notes)
• ✅ TEST_BASE_KEYBOARD_CURRENT_USER_KMX_EXISTS (PASSED) (notes)

Test Artifacts

• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• Keyboards
  • Test Keyboards
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows
  • Text Editor (ARM64)
  • Text Editor (x64)
  • Text Editor (x86)

[mcdurdin]

I don't think this is correct. The base keyboard dialog needs to run elevated -- because when you change the base keyboard, Keyman needs to mcompile each of the installed keyboards against the new base keyboard:

keyman/windows/src/engine/kmcomapi/com/options/keymanoptions.pas

Lines 133 to 135 in 1536a5f

	if FOldBaseLayout <> Get_Items('koBaseLayout').Value then
	for I := 0 to Context.Keyboards.Count - 1 do // I4169
	(Context.Keyboards.Items[I] as IIntKeymanKeyboardInstalled).UpdateBaseLayout;

The problem here is that the base layout setting is saved against the Admin user, but needs to be saved against the current user. This probably is best solved by splitting the admin component -- mcompiling -- out of the TKeymanOptions.Apply function, and running it as a separate step from the Base Keyboard dialog. Then the Base Keyboard dialog does not show elevated, but just elevates when OK is clicked, if it detects that new mcompiles need to be run, and does that as a kmshell -mcompile <basekeylayoutid> call? (implementation calls: TKeymanKeyboardInstalled.UpdateBaseLayout for each installed keyboard).

TKPRecompileMnemonicKeyboard then needs a parameter for the base layout, rather than reading it from the context options:

keyman/windows/src/engine/kmcomapi/processes/keyboard/kprecompilemnemonickeyboard.pas

Lines 87 to 88 in 9779792

	with Context as TKeymanContext do
	BaseKeyboardID := (Options as IKeymanOptions).Items['koBaseLayout'].Value;

So some plumbing required, sadly.

[rc-swag]

I don't think this is correct. The base keyboard dialog needs to run elevated -- because when you change the base keyboard, Keyman needs to mcompile each of the installed keyboards against the new base keyboard:

This change may not have been the prefered change but it worked. It does run as an elevated but the process is created as a current user.
I tested it last week and it changed the base keyboard for the non admin user. Not the admin. I will look into it again.

[mcdurdin]

Did you verify that this worked with a baselayout you had never selected previously? How could the mcompiled .kmx files be written to the C:\ProgramData folder if the base layout steps are run non-elevated?

One reasonably straightforward way to address this, for starting as a non-elevated user:

1. CU: The Base Keyboard dialog itself should not be elevated - launch from the current process.
2. CU: After clicking OK, verify if any mcompile is needed by looking to see if the needed .kmx files are already present in ProgramData.
3. CU: If mcompile is required, then wait for elevated kmshell -mcompile <baseklid> should be called to run the mcompile process.
4. LM: kmshell -mcompile will need to temporarily set the option[koBaseLayout] for the admin user in order to run the process. This will cause mcompile to be run. Reset option afterwards.
5. CU: After the wait for elevated process step completes, set option[koBaseLayout] for current user.

Also:

• If Keyman Configuration is started as an elevated user, skip steps 3 and 4.
• Need to update kmcomapi to only run mcompile if elevated

[mcdurdin]

5. Confirm the Keyboard changes for the current user and not the Admin user used for the elevated processs

Need to verify that the relevant files are saved in ProgramData too and that the base keyboard is mapped as expected!

[Meng-Heng]

Test Prerequisites

1. Setup a non-admin local user by following this https://www.wikihow.com/Create-a-New-Local-User-Account-in-Windows-10

Test Specs

1. Windows 10 Bootcamp

Test Results

• TEST_BASE_KEYBOARD_CURRENT_USER (PASSED):

1. Launch Keyman version 19.0.249
2. Open Keyman Configuration -> Options
3. Click Base Keyboard -> Enter Admin's password -> Change the Base Keyboard to English - Irish

Note

The Keyman Configuration window becomes unclickable until I click out of the app and click back. It does not show unresponding.

5. Confirmed: the Keyboard changes for the current user and not the Admin user.
6. To clarify, log into Admin account
7. Verified: The Base keyboard does not change to English - Irish.

Note

The 2nd and later attempts of changing the Base keyboard, this error pop up

The Base keyboard does not change until I click X, and then open Keyman again.

[mcdurdin]

The 2nd and later attempts of changing the Base keyboard, this error pop up

Note, if a crash dialog appears, FAIL the test. Also, please Copy to Clipboard and paste it into the test report.

TEST_BASE_KEYBOARD_CURRENT_USER (FAIL): a crash dialog appeared

[rc-swag]

@Meng-Heng
The tests need to be run again due the significant re-work of the changes.
Test-bot: retest TEST_BASE_KEYBOARD_CURRENT_USER TEST_BASE_KEYBOARD_CURRENT_USER_KMX_EXISTS

[mcdurdin]

I'll hold off reviewing until build passes 😀

[Meng-Heng]

Test Specs

1. Windows 10 Bootcamp
2. Keyman for Windows 19.0.268-alpha-test-16162

Test Results

• TEST_BASE_KEYBOARD_CURRENT_USER (PASSED):

1. Log into Standard non-admin user account
2. Launch Keyman -> Start Keyman -> Open Keyman Configuration
3. Install the sil_ipa keyboard within Keyman Configuration
4. Check C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa
5. Keep the sil_ipa.kmx file
6. Delete all of the KLIDs; sil_ipa-00000409-d.kmx...etc
7. Click Base Keyboard -> Change the Base Keyboard to German -> Enter Admin's credentials
8. VERIFIED: the Base Keyboard changes for the current user and not the Admin user.
9. Check C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa
10. VERIFIED: sil_ipa-00000407-d.kmx and sil_ipa-00000407.kmx appear.

• TEST_BASE_KEYBOARD_CURRENT_USER_KMX_EXISTS (PASSED):

1. After completing the steps in TEST_BASE_KEYBOARD_CURRENT_USER
2. Open Keyman Configuration -> Options
3. Click Base Keyboard -> Change the Base Keyboard to English - US -> Enter Admin's credentials
4. Close Configuration
5. VERIFIED: German mcomplied kmx is still there; C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipa, sil_ipa-00000407-d.kmx and sil_ipa-00000407.kmx
6. Open Keyman Configuration -> Options
7. Click Base Keyboard -> Change the Base Keyboard to German
8. VERIFIED: The admin credentials window pop-up does not appear.

[mcdurdin]

I am not comfortable with a BaseKeyboard variable and a FBaseKeyboard parameter -- too easy to confuse them! Can we perhaps refactor the fmBaseKeyboard case into a ConfigureAndSetBaseKeyboard function?

[mcdurdin]

Just some minor tweaks and a question about whether we need to publish a new public API for this? (I don't think the work is wasted effort, because you gained a deeper knowledge of how kmcomapi interfaces are implemented, but I am sorry if we decide that it really can be an internal interface)

[mcdurdin]

Let's make a function to build these filenames?

[rc-swag]

I not sure I follow what you mean, if you mean something like this? It doesn't add anything really for a function that is only called inside this loop.

function InsertBaseKeyboardIdFilename(const BaseFileName: string; BaseKeyboardIDHex: string; Deadkey: Boolean): string;
begin
  if Deadkey then
    Result := ChangeFileExt(BaseFileName, '') + '-' + BaseKeyboardIDHex + '-d.kmx'
  else
    Result := ChangeFileExt(BaseFileName, '') + '-' + BaseKeyboardIDHex + '.kmx';
end;

function BaseKeyboardNeedsMCompile(BaseKeyboardID: Integer): Boolean;
var
  I: Integer;
  Keyboard: IKeymanKeyboardInstalled;
  KeyboardFileName, BaseKeyboardIDFilename, BaseKeyboardIDDeadkeyFilename: string;
  BaseKeyboardIDHex: string;
begin
  BaseKeyboardIDHex := IntToHex(BaseKeyboardID, 8);
  for I := 0 to kmcom.Keyboards.Count - 1 do
  begin
    Keyboard := kmcom.Keyboards.Items[I];
    KeyboardFileName := Keyboard.Filename;
    BaseKeyboardIDFilename := InsertBaseKeyboardIdFilename(KeyboardFileName, BaseKeyboardIDHex, False);
    BaseKeyboardIDDeadkeyFilename := InsertBaseKeyboardIdFilename(KeyboardFileName, BaseKeyboardIDHex, True);

    if FileExists(KeyboardFileName) and
      (not FileExists(BaseKeyboardIDFilename) or
       not FileExists(BaseKeyboardIDDeadkeyFilename)) then
      Exit(True);
  end;
  Result := False;
end;

[mcdurdin]

I see UpgradeMnemonicLayout.pas has similar references. It'd be nice to group them so that we have a single set of functions that do the string manipulation.

[mcdurdin]

This then can also use ConfigureAndSetBaseKeyboard (from earlier comment)

[rc-swag]

Done

[mcdurdin]

I wonder if this could be slipped into IIntKeymanKeyboardInstalled and avoid publishing another interface?

[rc-swag]

I tried but kmshell needs to be able to make the call, I think.

[mcdurdin]

Yes, you are right, kmshell is a separate module to kmcomapi, so it needs to be a published API, not an Int(ernal) one. My bad, I even called that out in an earlier review. Sorry for the misdirection.

[mcdurdin]

LGTM, just asking for a move and rename of the new functions!

[mcdurdin]

I have made a number of suggestions to these functions. Can you move the whole lot to utilfiletypes.pas, rather than utilstr.pas?

[rc-swag]

Can you move the whole lot to utilfiletypes.pas, rather than utilstr.pas?

six for one half-a-dozen for the other. It isn't really filetype identifier but I guess it is making a new filename of the same type. It is also really str manipulation function. I will move it.