Fallback to UIA if supported in Chromium and we don't have access to IA2 #13032

LeonarddeR · 2021-11-04T11:15:01Z

Link to issue number:

None

Summary of the issue:

Currently, UIA is used in the following cases in Chromium (notably Edge)

It is force enabled
We aren't able to inject in process

There is a particular case missing here. When running Edge under a different user, the IAccessible2 implementation is not available, whereas UIA works pretty nicely.

Description of how this pull request fixes the issue:

Add another check to ensure we use UIA in this case.

Testing strategy:

Run edge under a different user and ensure that it is accessible.

Known issues with pull request:

None known

Change log entries:

Bug fixes

When running under a different user, Microsoft Edge is no longer inaccessible (i.e. by falling back to its UIA implementation).

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

seanbudd · 2021-11-07T23:19:21Z

When running Edge under a different user, the IAccessible2 implementation is not available

Do we know if this is intentional, and if so, why that is the case? Does Google Chrome have this behaviour? Maybe we should raise this with MS.

LeonarddeR · 2021-11-08T07:23:23Z

When running Edge under a different user, the IAccessible2 implementation is not available

Do we know if this is intentional, and if so, why that is the case? Does Google Chrome have this behaviour? Maybe we should raise this with MS.

Chrome has this behavior and so have Electron apps. Firefox is also not accessible with IA2 when running under a different user. I am under the impression that it might be an IA2 specific thing.
May be it helps if we add a function to NVDAHelperRemote that gives us an IAccessible pointer to work with, but I'm afraid that requires us to do all the IA2 work in process, which is a rediculous job for just this issue to address.

michaelDCurran · 2021-11-22T02:43:58Z

It would be good to be more clearer on the actual user scenarios here. What is the reasoning for running Edge as another user, and how does one actually do this?
I just logged in a s a standard user (not in the administrators group), Found Edge in the Start screen and chose Run as Administrator, and logged in as my normal (administrator group) account, and it worked fine.
No doubt this is not the scenario you were thinking of.
I do remember testing many mahy years back, that running an app as a different user caused a lot of things in NVDA not work work, including in-process injection, and even some of Microsoft's own MSAA code, such as successfully getting accName of a syslistview32 list item.

michaelDCurran · 2021-11-22T02:49:04Z

Calling accessibleObjectFromWindow is not a cheep call, nor probably is the accChild call.
I think I would prefer adding functionality to appModuleHandler.AppModule to query whether the process is running as a different user. This would be a lot more light-weight, only playing with the process token. and it would only need to be checked once and cached.

more information is needed

LeonarddeR · 2021-11-22T06:51:58Z

It would be good to be more clearer on the actual user scenarios here. What is the reasoning for running Edge as another user, and how does one actually do this? I just logged in a s a standard user (not in the administrators group), Found Edge in the Start screen and chose Run as Administrator, and logged in as my normal (administrator group) account, and it worked fine. No doubt this is not the scenario you were thinking of. I do remember testing many mahy years back, that running an app as a different user caused a lot of things in NVDA not work, including in-process injection, and even some of Microsoft's own MSAA code, such as successfully getting accName of a syslistview32 list item.

The scenario I use is

Logon to the system as a local administrator
Start Edge as a different user, shift+applications key on the shortcut
Logon as another user.

I'm working in a corporate environment where we use a different account for administrative tasks, such as database administratio. Running the browser under another user eases with single sign on.

Calling accessibleObjectFromWindow is not a cheep call, nor probably is the accChild call. I think I would prefer adding functionality to appModuleHandler.AppModule to query whether the process is running as a different user. This would be a lot more light-weight, only playing with the process token. and it would only need to be checked once and cached.

That makes sense. I'll close this and will think about another approach.

LeonarddeR · 2021-11-22T07:54:37Z

I implemented a new approach that gets the logon session ID from NVDA's process, caches that, and compares it with the logon session id of the appModule. Pretty sure that will be much cheaper indeed.
I still need to test this with your scenario, though.

LeonarddeR · 2021-11-22T08:14:46Z

I just logged in a s a standard user (not in the administrators group), Found Edge in the Start screen and chose Run as Administrator, and logged in as my normal (administrator group) account, and it worked fine.

I discussed this with a colleague and this indeed seems to differ significantly in that "run as admin" only ensures elevated permissions for the account that is currently running. So in your scenario, Edge would still be running as your standard user.
What happens if you do run as a different user and enter your admin credentials? I'm pretty sure the issue will occur in that case.

Also, while we are at it, could you may be elaborate on what the IA2support in NVDAHelper is supposed to do? Is that meant to initialize IA2 support for the helper process, or is it also necessary to initialize IA2 in such a way that out of process calls can use it?

michaelDCurran · 2021-11-22T22:31:03Z

Ah, I forgot about shift+applications key. Now I do see Run as user. I reproduced your problem by being logged in as a standard user, then running Edge as my other admin user. Everything just says "unknown".

michaelDCurran · 2021-11-22T22:32:59Z

Re IA2 in-process:
That code registers the IA2 proxy dll in that process, so that the IAccessible2 interfaces can be marshaled cross-process. In other words, to use IAccessible2 out-of-process, it is necessary to register it in-process. Though, code (like the gecko_ia2 virtualBuffers) would still work in theory.
However, since roughly 2018, Windows 10 has included the IAccessible2 proxy dll natively, with it registered system-wide. Thus making it unnecessary to register it in-process. They did this for their IA2 to UIA bridge. However, I really don't know how much they are keeping that up to date, so it is probably still necessary for us to register any newer interfaces, and of course on older Windows versions we need to register it all anyway.

source/systemUtils.py

LeonarddeR · 2021-11-23T06:44:10Z

Re IA2 in-process:

Thanks for clarifying.

I really want to get to the bottom of this somehow. It looks like the IA2 to UIA bridge. is perfectly able to expose stuff from Firefox as well, when that's running under a different user. On the other hand, NVDA is unable to read anything. This suggests there might be something permission wise that blocks us from accessing Firefox. As expected, when running NVDA as admin, all works perfectly fine.

seanbudd · 2022-02-16T23:09:16Z

I am marking this as a draft as this review comment is unresolved:

This call could fail due to not enough permissions on the process handle. Perhaps return None or raise an exception if it returns false

LeonarddeR · 2022-02-18T07:35:58Z

The review comment has been fixed.

AppVeyorBot · 2022-02-18T08:18:23Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ycgrss3e9byouui5/artifacts/output/nvda_snapshot_pr13032-24741,88fcb449.exe

See test results for failed build of commit 88fcb449fb

AppVeyorBot · 2022-06-21T10:02:08Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/y0ftqftgpiihq61o/artifacts/output/nvda_snapshot_pr13032-25659,f9ffef52.exe

See test results for failed build of commit f9ffef5225

AppVeyorBot · 2022-06-21T11:15:04Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/w377mvdt0i0cdatr/artifacts/output/nvda_snapshot_pr13032-25662,62109b1f.exe

See test results for failed build of commit 62109b1f8b

AppVeyorBot · 2022-06-21T18:16:13Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ktg4077jhmt04fmo/artifacts/output/nvda_snapshot_pr13032-25669,258c421f.exe

See test results for failed build of commit 258c421f9f

lukaszgo1 · 2022-06-21T18:43:30Z

source/nvda.pyw

@@ -77,6 +77,15 @@ import logHandler
 from logHandler import log
 import winUser
 import winKernel
+# Avoid a E402 'module level import not at top of file' warning,
+# because monkeypatches need to be applied first first


Is the second "first" a mistake, or have you added it just for emphasis?

That was definitely a mistake.

lukaszgo1 · 2022-06-21T18:51:03Z

source/nvda.pyw

+from systemUtils import getProcessTokenOrigin  # noqa: E402
+
+try:
+	globalVars.appLogonSessionID = getProcessTokenOrigin(winKernel.GetCurrentProcess())


An alternative implementation would be to create a\ function in systemUtils named getAppLoginID or similar which just retrieves logon ID of the current process first time it is called. If you're worried about performance you can cache the result similar to what is being done in hasSyswow64Dir. I see several advantages:

The NVDA's logon ID is retrieved only when needed i.e. only in cases where user accesses Chromium

In the current approach if there is an error when getting NVDA's logon ID we won't know about it since logging is not yet ready and exception is swallowed.

The NOQA comment would not be necessary :-D

I like this idea.

seanbudd

Thanks @LeonarddeR, the approach looks good to me. I've added some suggestions around coding style and documentation.

source/systemUtils.py

seanbudd · 2022-06-27T00:38:12Z

source/systemUtils.py

+	_fields_ = [("OriginatingLogonSession", ctypes.c_ulonglong)]
+
+
+def getProcessTokenOrigin(processHandle: int) -> TOKEN_ORIGIN:


A docstring describing what this is getting, and what it is used for is needed here.

I decided to merge the approach to get the logon session ID in one function, it looks more obvious now.

AppVeyorBot · 2022-06-28T06:48:50Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/upovjowcn8mwgtu5/artifacts/output/nvda_snapshot_pr13032-25703,b7596ca0.exe

See test results for failed build of commit b7596ca0e1

seanbudd · 2022-07-12T05:20:39Z

Is this still meant to be a draft? What's remaining here?

LeonarddeR · 2022-07-12T05:52:59Z

Ah I'm sorry, I think it's ready now.

seanbudd

Thanks @LeonarddeR

Fallback to UIA if supported in Chromium and we don't have access to IA2

38f7a14

LeonarddeR requested a review from a team as a code owner November 4, 2021 11:15

LeonarddeR requested a review from seanbudd November 4, 2021 11:15

Lint

8a052c3

seanbudd previously approved these changes Nov 22, 2021

View reviewed changes

seanbudd requested a review from michaelDCurran November 22, 2021 01:56

LeonarddeR closed this Nov 22, 2021

LeonarddeR reopened this Nov 22, 2021

LeonarddeR added 2 commits November 22, 2021 08:46

Merge remote-tracking branch 'origin/master' into chromiumUIA

b654bc6

Use different approach based on logon session

56c328a

michaelDCurran reviewed Nov 22, 2021

View reviewed changes

source/systemUtils.py Outdated Show resolved Hide resolved

seanbudd marked this pull request as draft February 16, 2022 23:09

LeonarddeR added 2 commits February 18, 2022 08:22

Merge remote-tracking branch 'origin/master' into chromiumUIA

fc71681

Review actions

f141828

LeonarddeR marked this pull request as ready for review February 18, 2022 07:35

Fix linter

3cb5a32

Linter fix

d1d69ff

Fix importing

5afa213

lukaszgo1 reviewed Jun 21, 2022

View reviewed changes

Review actions

07997e8

seanbudd self-assigned this Jun 22, 2022

lukaszgo1 approved these changes Jun 22, 2022

View reviewed changes

seanbudd reviewed Jun 27, 2022

View reviewed changes

seanbudd removed the blocked/needs-code-review label Jun 27, 2022

seanbudd marked this pull request as draft June 27, 2022 01:06

seanbudd added the merge-early Merge Early in a developer cycle label Jun 28, 2022

LeonarddeR added 4 commits June 28, 2022 07:45

Merge remote-tracking branch 'origin/master' into chromiumUIA

cc91ae6

COmments and naming style

12b46e9

TOKEN_ORIGIN is a constant

e53911c

Doc string

ee0e687

LeonarddeR marked this pull request as ready for review July 12, 2022 05:52

LeonarddeR added 2 commits July 12, 2022 07:54

Linter

5c92632

Merge remote-tracking branch 'origin/master' into chromiumUIA

de03806

seanbudd self-requested a review July 12, 2022 06:04

seanbudd added the blocked/needs-code-review label Jul 12, 2022

seanbudd approved these changes Jul 13, 2022

View reviewed changes

seanbudd added 2 commits July 13, 2022 12:07

Merge branch 'master' into chromiumUIA

062745a

update changes

466153a

seanbudd merged commit a09aef3 into nvaccess:master Jul 13, 2022

nvaccessAuto added this to the 2022.3 milestone Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to UIA if supported in Chromium and we don't have access to IA2 #13032

Fallback to UIA if supported in Chromium and we don't have access to IA2 #13032

LeonarddeR commented Nov 4, 2021 •

edited

Loading

seanbudd commented Nov 7, 2021

LeonarddeR commented Nov 8, 2021

michaelDCurran commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

LeonarddeR commented Nov 23, 2021

seanbudd commented Feb 16, 2022

LeonarddeR commented Feb 18, 2022

AppVeyorBot commented Feb 18, 2022

AppVeyorBot commented Jun 21, 2022

AppVeyorBot commented Jun 21, 2022

AppVeyorBot commented Jun 21, 2022

lukaszgo1 Jun 21, 2022

LeonarddeR Jun 22, 2022

lukaszgo1 Jun 21, 2022

LeonarddeR Jun 22, 2022

seanbudd left a comment

seanbudd Jun 27, 2022

LeonarddeR Jun 28, 2022

AppVeyorBot commented Jun 28, 2022

seanbudd commented Jul 12, 2022

LeonarddeR commented Jul 12, 2022

seanbudd left a comment

		_fields_ = [("OriginatingLogonSession", ctypes.c_ulonglong)]


		def getProcessTokenOrigin(processHandle: int) -> TOKEN_ORIGIN:

Fallback to UIA if supported in Chromium and we don't have access to IA2 #13032

Fallback to UIA if supported in Chromium and we don't have access to IA2 #13032

Conversation

LeonarddeR commented Nov 4, 2021 • edited Loading

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

seanbudd commented Nov 7, 2021

LeonarddeR commented Nov 8, 2021

michaelDCurran commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

LeonarddeR commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

michaelDCurran commented Nov 22, 2021

LeonarddeR commented Nov 23, 2021

seanbudd commented Feb 16, 2022

LeonarddeR commented Feb 18, 2022

AppVeyorBot commented Feb 18, 2022

AppVeyorBot commented Jun 21, 2022

AppVeyorBot commented Jun 21, 2022

AppVeyorBot commented Jun 21, 2022

lukaszgo1 Jun 21, 2022

Choose a reason for hiding this comment

LeonarddeR Jun 22, 2022

Choose a reason for hiding this comment

lukaszgo1 Jun 21, 2022

Choose a reason for hiding this comment

LeonarddeR Jun 22, 2022

Choose a reason for hiding this comment

seanbudd left a comment

Choose a reason for hiding this comment

seanbudd Jun 27, 2022

Choose a reason for hiding this comment

LeonarddeR Jun 28, 2022

Choose a reason for hiding this comment

AppVeyorBot commented Jun 28, 2022

seanbudd commented Jul 12, 2022

LeonarddeR commented Jul 12, 2022

seanbudd left a comment

Choose a reason for hiding this comment

LeonarddeR commented Nov 4, 2021 •

edited

Loading