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

Remove SecureDesktopNVDAObject, replace with extension point #14488

Merged
merged 10 commits into from Sep 7, 2023

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Dec 29, 2022

Link to issue number:

Fixes #14137

Summary of the issue:

SecureDesktopNVDAObject is used to notify the user and API consumers that NVDA has entered a secure desktop.
This creates a valid NVDAObject, backed by the secure desktop.
The running instance of NVDA enters sleep mode when SecureDesktopNVDAObject is focused, signifying a switch to a secure desktop. Then a new instance of NVDA in secure mode starts on the secure desktop.
The SecureDesktopNVDAObject is unused, apart from being an API endpoint to notify consumers, as NVDA sleeps after it is created.

Handling SecureDesktopNVDAObject requires special cases to be constructed.

NVDA generally uses extension points for this API use case.

Description of user facing changes

None

Description of development approach

SecureDesktopNVDAObject is replaced with an extension point that only provides a boolean state on the existence of the secure desktop.
A fake NVDAObject is used to enable sleep mode while the secure desktop is active.

Testing strategy:

Smoke test the following screens, ensure "Secure Desktop" is announced:

  • UAC dialog
  • sign-in screen

Known issues with pull request:

None

Change log entries:

Refer to diff

  • Announce API breaking change on add-on API mailing list

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
  • Security precautions taken.

@seanbudd seanbudd requested a review from a team as a code owner December 29, 2022 03:57
@seanbudd seanbudd requested review from michaelDCurran and removed request for feerrenrut December 29, 2022 03:58
@seanbudd seanbudd added this to the 2023.1 milestone Dec 30, 2022
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering...
Since we are taking the time to refactor this, it may be worth considering other non-secure desktop switch scenarios also. See #14395 for example.
Perhaps we should rename things a bit so it is not so security oriented, rather instead is is more about NVDA's desktop activating / deactivating.
So for example, when switching to a secure desktop, what is more broadly happening is that the Desktop NvDA is running on is no longer the active input desktop. Thus NvDA should notify code about this, (possibly announce something to the user) and go into sleep mode.
Similarly, when switching back from a secure desktop, more broadly, the desktop NvDA is running on has again become the active input desktop, and therefore code should be informed and sleep mode should be disabled and the current focus should be reported.
These examples could very easily be changed to the scenario from #14395 where a second user desktop has been switched to. In this case it is not a secure desktop, but all the same, NVDA should still notify and go to sleep. Separate to this pr, we may want a second NVDA to start on the other user desktop, similar to how we start on a secure desktop, but that is future work.

source/winAPI/secureDesktop.py Show resolved Hide resolved
@michaelDCurran
Copy link
Member

michaelDCurran commented Jan 10, 2023 via email

@michaelDCurran
Copy link
Member

As this pr provides no user visible fix or enhancement, yet does clearly cause API breakage for at least one known add-on, and seems that it would be the largest breaking change if merged, I would like to hold this back until 2024.1 as we shall be upgrading Python in that release and many add-ons will be affected. I'd prefer to do that all in one go.

@seanbudd seanbudd merged commit c3e095e into master Sep 7, 2023
1 check passed
@seanbudd seanbudd deleted the remove-SecureDesktopNVDAObject branch September 7, 2023 04:17
seanbudd pushed a commit that referenced this pull request Sep 8, 2023
…tateChange extensionPoint (#15392)

Fix-up of #14488

Summary of the issue:
In #14488, no documentation for the new extensionPoint was added to the master list of extension points in the developer guide.
(I wish there was a way to remind of this when contributing extensionPoint creations)

Additionally, I noticed an oddity in the documentation of the extensionPoint itself, which may or may not be intended.

Description of user facing changes
Improved documentation for developers.

Description of development approach
Added a new section to the Developer Guide chapter on Extension Points, listing the new Action, winAPI.secureDesktop.post_secureDesktopStateChange.
Fixed a terminology error in the head section of the chapter (I had previously called extensionPoints a module, when it should have been a package).
Noticed that in the usage example given in the winAPI.secureDesktop module docstring, there was a call to notify(). AFAIK, the general consumer of this extensionPoint, who registers something to it, shouldn't also be calling notify on it. I removed that example line, replacing it with a comment to put the subsequent unregister() call in context. @seanbudd please check my reasoning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for SecureDesktopNVDAObject, replace with an extension point
2 participants