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

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

Merged
merged 6 commits into from
Mar 28, 2022

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Mar 28, 2022

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
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 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.

@AppVeyorBot
Copy link

See test results for failed build of commit a7325c6036

@AppVeyorBot

This comment was marked as off-topic.

@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
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants