Copying settings to secure screens gives a warning about addons even when they are all disabled.

feerrenrut commented 6 years ago

Steps to reproduce:

Ensure that you have one or more addons installed, but all disabled.
Open the General settings dialog (NVDA+control+g).
Ensure that "Use NVDA on the Windows logon screen (requires administrator privileges)" is checked
Press the "Use currently saved settings on the logon and other secure screens (requires administrator privileges)" button.

Expected behavior:

No warnings

Actual behavior:

The following dialog is shown: "Warning dialog Add-ons were detected in your user settings directory. Copying these to the system profile could be a security risk. Do you still wish to copy your settings?"

Discussion:

There is no way to enable addons from the log on screen, so copying these addons is not useful. This should be updated so only enabled addons are copied, and the warning message lists which addons will be copied to make it easier for the user to make an informed decision. When there are no enabled addons, none should be copied, and there should be no warning message.

System configuration:

NVDA version: next-15060,4709bfa7

NVDA Installed or portable: Installed

Windows version: Windows 10 Version 1709

josephsl commented 6 years ago

Hi, I think one way to get around that is listing installed add-ons and let users choose which ones to copy. CC @DerekRiemer

Brian1Gaff commented 6 years ago

I'd have thought this was OK as presumably the add ons are still copied even though their state is off.

Brian

nvdaes commented 6 years ago

I'm creating a PR, just checking if all add-ons are disabled and, in this case, it won't be necessary checking if add-ons are detected in the user configuration. The problem is that all pluggins could be copied and they would take effect in secure screens. Ideally, NVDA should remove definitely old pluggins like global plugins, app modules or drivers, and deal just with add-ons. Also, we can check and provide info about running add-ons, but this doesn't include globalPlugins, or it would be tricky, since they can be just Python files or even folders inside global plugins, app modules or other drivers. When add-ons are disabled, these plugins don't work, but they would work in secure screens.

awalvie commented 5 years ago

Hey there, I am trying to work on the issue and was looking at the PR, can someone let me know why the PR was not merged so maybe I can fix the problem ?

feerrenrut commented 5 years ago

@Awalvie please read the PR carefully. There are many outstanding issues, the final comment does a pretty good job of summarising them. There are two aspects to this problem, and I think we can accept them as individual PRs:

The first would be the simpler approach, but would probably be made redundant by the second, and solves part of the problem but not all of it. When there are no addons, or all addons are disabled (either by the --disable-addons flag, or because all are individually disabled) then don't warn the user.
The second is to provide a checkable list to allow the user to select which addon-ons (as well as "all" and "none") should be copied.

Other ideas that need work in this area:

Ensure that the user config is first cleared. It should not merely add new settings. Ideally this would not increase the time it takes, I would suggest using a tool like robocopy to mirror the directories.
Ensure that only the required parts of the config should be copied.

awalvie commented 5 years ago

Alright, I am mostly a beginner so I think I should start with working on the first part of the problem and then work on the next part. Thanks for explaining the problem to me I'll try to get this implemented.

awalvie commented 5 years ago

Quick question ? When I try to compile the code from source and try to replicate the problem I don't get the administrator privileges required to enable step 3. I tried asking on the mailing list but I am having some problems with my subscription to it, so against my better judgement I decided to post here so I can test my changes, sorry if this is not the right place to be posting this question.

josephsl commented 5 years ago

Hi, source code copies have restrictions like this. Thanks.

awalvie commented 5 years ago

I appreciate the reply but can you also tell me how to enable the admin rights so I can replicate the issue ?

josephsl commented 5 years ago

Hi, no, sorry, this can’t be done for source code copies. Thanks.

awalvie commented 5 years ago

So I cannot replicate the issue then ?

josephsl commented 5 years ago

Hi, no, not with the source code copy. Thanks.

awalvie commented 5 years ago

Then how am I supposed to work on it ?

josephsl commented 5 years ago

Hi,

One risky way is modifying source/gui/settingsDialogs.py and removing security checks around general settings dialog class. Doing so means:

Edit appropriate sections of the source code.
Undo your own edits about security checks.

I think, at this point, it'd be best to ask @NVDAEs and others who have directly worked with that PR to keep working on it, with feedback here and there.

Thanks.

awalvie commented 5 years ago

Alright, thanks for the input.

josephsl commented 5 years ago

Hi,

I'd so far as remove "good for new dev" label, as this is not that trivial (as explained today).

Thanks.

lukaszgo1 commented 5 years ago

In my view the easiest way of testing and then fixing this would be to generate your own certificate as described here then installing self-signed build with your changes and testing from there. Regarding removing GoodForNewDev I personally disagree if new developers wouldn't be attempting to fix harder issues how they are supposed to work on something without this label.

josephsl commented 5 years ago

Hi, at least for local changes, the certificate method will work. A try build is also useful for this kind of thing. Thanks

feerrenrut commented 5 years ago

@Awalvie

I tried asking on the mailing list but I am having some problems with my subscription to it,

We have become aware of outages affecting the mailing list infrastructure. The developers mailing list has now been moved to https://nvda-devel.groups.io/

awalvie commented 5 years ago

Yep, just joined it. Would you recommend unsubscribing from the older list ?

feerrenrut commented 5 years ago

No, I would stay subscribed to the old list, I doubt there could be any harm in staying subscribed. I'm going to mark these messages as off topic, since this is not really an appropriate place to discuss them. Gitter is a good way to get faster feedback for small issues you might be having with NVDA development.

josephsl commented 5 years ago

Hi,

T @NVDAEs, can you rebase the companion PR on master (Python 3) if you are comfortable with doing so?

Thanks.

nvaccess / nvda