Skip to content

Show reset config as a menu item in secure mode#13547

Merged
seanbudd merged 6 commits into
masterfrom
allowRevertConfigSecure
Mar 28, 2022
Merged

Show reset config as a menu item in secure mode#13547
seanbudd merged 6 commits into
masterfrom
allowRevertConfigSecure

Conversation

@seanbudd

@seanbudd seanbudd commented Mar 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

None

Summary of the issue:

Currently a user can temporarily reset to factory defaults in secure mode by pressing the default input gesture NVDA+control+r 3 times.
A user using NVDA in a shared kiosk may find this command helpful to temporarily use NVDA with factory settings, so that they can log in and then use their own settings profile.

This is safe, as resetting to factory defaults doesn't save the settings profile to disk. Users can revert to the saved configuration for secure screens by:

  • using the menu
  • NVDA+control+r
  • restarting NVDA

The feature "reset configuration to factory defaults" is hidden from the NVDA menu in secure mode, even though it is exposed through a default input gesture.

Description of how this pull request fixes the issue:

Show "reset configuration to factory defaults" as a menu item in secure mode

Testing strategy:

Manual testing:

  • Change a preference in NVDA preferences
  • Copy your configuration to secure screens
  • From a secure screen
    • Note that the menu item is visible
    • Close the preference dialog
    • Reset configuration to factory defaults using the menu item
    • Confirm that the changed preference has been reset to the factory default
  • Restart the machine
  • on the sign-in screen, confirm that the preference has been restored to the saved config - not the factory default

Known issues with pull request:

None

Change log entries:

Bug fixes

- "Reset configuration to factory defaults" is now accessible in the NVDA menu during secure mode.

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 seanbudd requested a review from a team as a code owner March 28, 2022 01:42
@seanbudd seanbudd requested a review from michaelDCurran March 28, 2022 01:42
michaelDCurran
michaelDCurran requested changes Mar 28, 2022
View reviewed changes

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think appendConfigProfilesSection would be better named something like appendConfigManagementSection. Server, reset and save are to do with configuration, but not specifically to do with Config profiles.
Also, I think the PR description / tests should clearly mention that resetting to factory defaults is safe as we don't actually save the config on secure mode. The tests should mention resetting to factory defaults, confirming that the reset worked, and then exiting the secure desktop / restarting the machine, and confirming that the config was back to what it was before resetting to factory defaults.

@seanbudd seanbudd requested a review from michaelDCurran March 28, 2022 02:08
@AppVeyorBot

AppVeyorBot commented Mar 28, 2022

Copy link
Copy Markdown

See test results for failed build of commit a7325c6036

@AppVeyorBot

This comment was marked as off-topic.

Sign in to view
@seanbudd seanbudd merged commit ad5666b into master Mar 28, 2022
@seanbudd seanbudd deleted the allowRevertConfigSecure branch March 28, 2022 06:06
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 28, 2022
@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Milestone

2022.2

Development

Successfully merging this pull request may close these issues.

5 participants

@seanbudd @AppVeyorBot @michaelDCurran @feerrenrut @nvaccessAuto