fix(windows): set basekeyboard as current user not the admin user on a elevated process. - #16162
fix(windows): set basekeyboard as current user not the admin user on a elevated process.#16162rc-swag wants to merge 20 commits into
Conversation
User Test ResultsTest specification and instructions
Test Artifacts |
mcdurdin
left a comment
There was a problem hiding this comment.
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
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:
So some plumbing required, sadly.
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. |
|
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:
Also:
|
Need to verify that the relevant files are saved in ProgramData too and that the base keyboard is mapped as expected! |
Test Prerequisites
Test Specs
Test Results
Note The Keyman Configuration window becomes unclickable until I click out of the app and click back. It does not show unresponding.
|
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 |
2d6a4ee to
2c48025
Compare
| @@ -152,14 +156,19 @@ procedure TKeymanKeyboardInstalled.Uninstall; | |||
| end; | |||
|
|
|||
| procedure TKeymanKeyboardInstalled.UpdateBaseLayout; // I4169 | |||
There was a problem hiding this comment.
This could now be removed
There was a problem hiding this comment.
@mcdurdin how do I go about deprecating this as it is in a interface.
There was a problem hiding this comment.
This is only in an internal interface (IIntKeymanKeyboardsInstalled) so we can just remove it from the interface. It's not a published API. It's not even used outside of kmcomapi, which makes it pretty clean. All the interfaces in internalinterfaces.pas are used only in kmcomapi which makes it safe to modify them as needed -- no need to changing GUIDs or any other files.
(If the interface had been used cross-module, then you'd regenerate the GUID to make it easier to track errors -- resulting in 'interface not found' rather than a weird and hard to diagnose crash.)
4 results - 3 files
windows\src\engine\kmcomapi\com\keyboards\keymankeyboardinstalled.pas:
111 procedure ClearVisualKeyboard;
112: procedure UpdateBaseLayout; // I4169
113 procedure RefreshInstallation;
153
154: procedure TKeymanKeyboardInstalled.UpdateBaseLayout; // I4169
155 begin
windows\src\engine\kmcomapi\com\options\keymanoptions.pas:
134 for I := 0 to Context.Keyboards.Count - 1 do // I4169
135: (Context.Keyboards.Items[I] as IIntKeymanKeyboardInstalled).UpdateBaseLayout;
136
windows\src\engine\kmcomapi\util\internalinterfaces.pas:
69 procedure ClearVisualKeyboard;
70: procedure UpdateBaseLayout; // I4169
71 procedure RefreshInstallation;
The updates all the apis so that the basekeyboardid or klid can be passed in as an argument. This in needed so that elevated process required to compile the keyboard has the call users keyboard base id.
|
@Meng-Heng |
|
I'll hold off reviewing until build passes 😀 |
Test Specs
Test Results
|
| else if s = '-basekeyboard' then FMode := fmBaseKeyboard // I4169 | ||
| else if s = '-mcompilekbds' then | ||
| begin | ||
| FMode := fmMCompileKbds; |
There was a problem hiding this comment.
| FMode := fmMCompileKbds; | |
| // Requires elevated context | |
| FMode := fmMCompileKbds; |
It may be good for each command to denote if it requires elevated context in a programmatic way in the future. I think we have scope to separate internal commands and external ones as well -- for example, this is really an internal-use command with little scope generally for external use.
| kdl: IKeymanDefaultLanguage; | ||
| FIcon: string; | ||
| FMutex: TKeymanMutex; // I2720 | ||
| BaseKeyboardID: Integer; |
There was a problem hiding this comment.
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?
| @@ -0,0 +1,95 @@ | |||
| unit Keyman.Configuration.System.BaseKeyboard; | |||
mcdurdin
left a comment
There was a problem hiding this comment.
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)
| (** | ||
| Returns true if the keyboard files need to be compiled for the specified KLID. | ||
| @param BaseKeyboardID KLID of the base keyboard to compile. | ||
| @returns True If the keyboard files need to be compiled. | ||
| *) |
There was a problem hiding this comment.
| (** | |
| Returns true if the keyboard files need to be compiled for the specified KLID. | |
| @param BaseKeyboardID KLID of the base keyboard to compile. | |
| @returns True If the keyboard files need to be compiled. | |
| *) | |
| (** | |
| * Returns true if the keyboard files need to be compiled for the specified KLID. | |
| * @param BaseKeyboardID KLID of the base keyboard to compile. | |
| * @returns True If the keyboard files need to be compiled. | |
| *) |
| (** | ||
| Sets the base keyboard KLID for the current user and compiles the keyboard | ||
| files if necessary. In the case the compiled keyboard files are not present, | ||
| it will require elevation. | ||
| @param WindowHandle Window handle to own the elevation prompt. | ||
| @param BaseKeyboardID KLID of the base keyboard KLID to set. | ||
| @returns True when the base keyboard setting has been applied. | ||
| *) |
There was a problem hiding this comment.
| (** | |
| Sets the base keyboard KLID for the current user and compiles the keyboard | |
| files if necessary. In the case the compiled keyboard files are not present, | |
| it will require elevation. | |
| @param WindowHandle Window handle to own the elevation prompt. | |
| @param BaseKeyboardID KLID of the base keyboard KLID to set. | |
| @returns True when the base keyboard setting has been applied. | |
| *) | |
| (** | |
| * Sets the base keyboard KLID for the current user and compiles the keyboard | |
| * files if necessary. In the case the compiled keyboard files are not present, | |
| * it will require elevation. | |
| * @param WindowHandle Window handle to own the elevation prompt. | |
| * @param BaseKeyboardID KLID of the base keyboard KLID to set. | |
| * @returns True when the base keyboard setting has been applied. | |
| *) |
| (** | ||
| Compiles the base keyboard files for the specified KLID. | ||
| @param BaseKeyboardID KLID of the base keyboard to compile. | ||
| @returns True when the compilation is successful. | ||
| *) |
There was a problem hiding this comment.
| (** | |
| Compiles the base keyboard files for the specified KLID. | |
| @param BaseKeyboardID KLID of the base keyboard to compile. | |
| @returns True when the compilation is successful. | |
| *) | |
| (** | |
| * Compiles the base keyboard files for the specified KLID. | |
| * Must run elevated. | |
| * | |
| * @param BaseKeyboardID KLID of the base keyboard to compile. | |
| * @returns True when the compilation is successful. | |
| *) |
| (not FileExists(ChangeFileExt(BaseFileName, '') + '-' + BaseKeyboardIDHex + '.kmx') or | ||
| not FileExists(ChangeFileExt(BaseFileName, '') + '-' + BaseKeyboardIDHex + '-d.kmx')) then |
There was a problem hiding this comment.
Let's make a function to build these filenames?
| (** | ||
| Form for the user to select a base keyboard. If the user selects a base | ||
| keyboard, the KLID of the selected base keyboard is returned in | ||
| BaseKeyboardID. | ||
| @param [out] BaseKeyboardID KLID of the base keyboard selected by the user. | ||
| @returns True if the user selected a base keyboard. | ||
| *) |
There was a problem hiding this comment.
| (** | |
| Form for the user to select a base keyboard. If the user selects a base | |
| keyboard, the KLID of the selected base keyboard is returned in | |
| BaseKeyboardID. | |
| @param [out] BaseKeyboardID KLID of the base keyboard selected by the user. | |
| @returns True if the user selected a base keyboard. | |
| *) | |
| (** | |
| * Form for the user to select a base keyboard. If the user selects a base | |
| * keyboard, the KLID of the selected base keyboard is returned in | |
| * BaseKeyboardID. | |
| * @param [out] BaseKeyboardID KLID of the base keyboard selected by the user. | |
| * @returns True if the user selected a base keyboard. | |
| *) |
| var | ||
| BaseKeyboardID: Integer; | ||
| begin | ||
| WaitForElevatedConfiguration(Handle, '-basekeyboard'); | ||
| // Refresh will be triggered by elevated process | ||
| if ConfigureBaseKeyboard(BaseKeyboardID) then | ||
| begin | ||
| SetBaseKeyboard(Handle, BaseKeyboardID); | ||
| DoRefresh; | ||
| end; |
There was a problem hiding this comment.
This then can also use ConfigureAndSetBaseKeyboard (from earlier comment)
| procedure RefreshInstallation; | ||
|
|
||
| { IKeymanKeyboardInstalled2 } | ||
| procedure MCompileForBaseKeyboard(KLID: Integer); safecall; |
There was a problem hiding this comment.
I wonder if this could be slipped into IIntKeymanKeyboardInstalled and avoid publishing another interface?

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 is already compiled keyboards for the selected basekeyboard. If there aren't compiled keyboards in then elevates to compile the keyboards but passes in the current users 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
Login into Windows with and account that is a "standard" user and does not have "Administrator" rights.
Install the Keyman from this PR
Open Keyman Configuration -> Keyboard Layouts
Install a keyboard for example sil_ipa
Open Keyman Configuration -> Options
Check
C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipadon't delete the basesil_ipa.kmxbut delete any with KLIDs for examplesil_ipa-00000409-d.kmxClick Base Keyboard, change the Base Keyboard to German. You will need to enter the login details for a Admin user.
Confirm the Keyboard changes for the current user and not the Admin user used for the elevated processs.
Check
C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipathere should now be asil_ipa-????0407-d.kmxandsil_ipa-????0407.kmxTEST_BASE_KEYBOARD_CURRENT_USER_KMX_EXISTS
After completing the steps in
TEST_BASE_KEYBOARD_CURRENT_USERCheck German mcomplied kmx is still there i.e.
C:\ProgramData\Keyman\Keyman Engine\Keyboard\_Package\sil_ipathere should now be asil_ipa-????0407-d.kmxandsil_ipa-????0407.kmx6.Click Base Keyboard, change the Base Keyboard to German. You should Not be asked to enter a admin user.