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

[GPO; Enterprise] Updater policies #24221

Merged
merged 30 commits into from
Feb 24, 2023
Merged

Conversation

htcfreek
Copy link
Collaborator

@htcfreek htcfreek commented Feb 20, 2023

Summary of the Pull Request

This PR adds the following three updater policies:

  • Disable automatic download.
  • Suspend toast notification from periodic check for two minor releases.
  • Disable automatic background update check. (This GPO will be implemented for later usage, but not active.)

image

image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

TESTED:

  • Disable periodic update GPO.
  • Disable automatic download GPO.

NOT TESTED (Not possible for me.):

  • Suspend toast notification GPO.

@htcfreek
Copy link
Collaborator Author

@snickler, @jaimecbernardo
I get this error when building locally:

Severity	Code	Description	Project	File	Line	Suppression State
Error	C1083	Cannot open include file: 'GPOWrapper.g.h': No such file or directory (compiling source file UpdateUtils.cpp)	runner	D:\AppDev\Github\htcfreek\PT_UpdateGpo\src\common\GPOWrapper\GPOWrapper.h	2	

Do you have an idea why?

@snickler
Copy link
Collaborator

@snickler, @jaimecbernardo
I get this error when building locally:

Severity	Code	Description	Project	File	Line	Suppression State
Error	C1083	Cannot open include file: 'GPOWrapper.g.h': No such file or directory (compiling source file UpdateUtils.cpp)	runner	D:\AppDev\Github\htcfreek\PT_UpdateGpo\src\common\GPOWrapper\GPOWrapper.h	2	

Do you have an idea why?

I would say to try running the powershell to remove the generated directories and build again using msbuild. I will also try building to see if I can repro

@htcfreek
Copy link
Collaborator Author

@snickler, @jaimecbernardo
I get this error when building locally:

Severity	Code	Description	Project	File	Line	Suppression State
Error	C1083	Cannot open include file: 'GPOWrapper.g.h': No such file or directory (compiling source file UpdateUtils.cpp)	runner	D:\AppDev\Github\htcfreek\PT_UpdateGpo\src\common\GPOWrapper\GPOWrapper.h	2	

Do you have an idea why?

I would say to try running the powershell to remove the generated directories and build again using msbuild. I will also try building to see if I can repro

Is it auto generated? I can't find it in the folder structure.

@snickler
Copy link
Collaborator

@snickler, @jaimecbernardo
I get this error when building locally:

Severity	Code	Description	Project	File	Line	Suppression State
Error	C1083	Cannot open include file: 'GPOWrapper.g.h': No such file or directory (compiling source file UpdateUtils.cpp)	runner	D:\AppDev\Github\htcfreek\PT_UpdateGpo\src\common\GPOWrapper\GPOWrapper.h	2	

Do you have an idea why?

I would say to try running the powershell to remove the generated directories and build again using msbuild. I will also try building to see if I can repro

Is it auto generated? I can't find it in the folder structure.

Yup it's generated. Oh, also nuke all Msbuild.exe and VBCSCompiler.exe instances before you try again

@snickler
Copy link
Collaborator

Hmmm I'm unsure why this is failing to build properly. I want to say it could be something not matching with the changes that it's failing to create the generated file needed. I'll dig in and see what I can find.

@snickler
Copy link
Collaborator

@htcfreek - Oh, I think I know what it is. It's likely because of UpdateUtils directly including the GPOWrapper.h. Since that GPOWrapper.g.h it's referencing is generated during the build of the GPOWrapper project, it's probably not going to be in existence at the time of it being referenced by the updater. There may be some compilation based things that have to do or maybe some ifdefs around including GPOWrapper.g.h? I'm not 100% sure.

@htcfreek
Copy link
Collaborator Author

@snickler
I finish the PR and then @jaimecbernardo can hopefully help me with fixing the build. It is my first time coding C++, so I don't know how to fix it.

@snickler
Copy link
Collaborator

@snickler
I finish the PR and then @jaimecbernardo can hopefully help me with fixing the build. It is my first time coding C++, so I don't know how to fix it.

Ha, I think I fixed it.

@htcfreek
Copy link
Collaborator Author

@snickler
I finish the PR and then @jaimecbernardo can hopefully help me with fixing the build. It is my first time coding C++, so I don't know how to fix it.

Ha, I think I fixed it.

Nice. Can you tell me how or push the fix?

Added relative path to Generated Files folder for GPOWrapper.g.h
@snickler
Copy link
Collaborator

@snickler
I finish the PR and then @jaimecbernardo can hopefully help me with fixing the build. It is my first time coding C++, so I don't know how to fix it.

Ha, I think I fixed it.

Nice. Can you tell me how or push the fix?

I just pushed the change to it that worked locally for me. I added the Generated Files path to the include and that seemed to be fine

@snickler
Copy link
Collaborator

Looks like the arm64 build completed successfully. I'm sure there's probably a BETTER way of doing this. I wonder if it would be better to add the Generated Files folder for GPOWrapper into the additional include directories for the runner project rather than hardcoding the directory name in GPOWrapper.h? @jaimecbernardo thoughts?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Feb 21, 2023

@niels9001, @stefansjfw , @snickler I investigated the visibility property (IsAdmin) of the AutoDownload settings card and found out that it his hidden for normal users who not a member of the Admin group. Because we normal only disable the card with an info bar, we now have a GPO and because I don't see any reason why we should block normal users from changing AND SEEING this setting, I decided to improve UI here. I will do this in this PR because I don't like to do things twice.

I have check the issue and PR history. It seems that we decide to not show the setting to normal users, because they should not enable it. We don't want this because the can't install the updates. (See also #2885.)

@htcfreek htcfreek marked this pull request as ready for review February 21, 2023 12:10
src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved
src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved
@htcfreek
Copy link
Collaborator Author

@jaimecbernardo , @stefansjfw
Feedback is addressed

get => !AutoUpdatesDisabledOnDevBuild && !_autoDownloadUpdatesIsGpoDisabled;
}

// The settings card is hidden for users who are not a member of the Administrators group and in this case the GPO info should be hidden too.
Copy link
Collaborator

Choose a reason for hiding this comment

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

note that this will be changed once per-user installer is out

Copy link
Collaborator Author

@htcfreek htcfreek Feb 23, 2023

Choose a reason for hiding this comment

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

Okay. But then we need to update it after user install is out. I only improved code.

Currently it is hidden so gpo message should be too.

And btw, I think this will be related to the install mode. If PT is installed as admin a user still has no permissions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, yeah, it was note to myself mostly :)

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

lgtm

@htcfreek
Copy link
Collaborator Author

htcfreek commented Feb 23, 2023

@stefansjfw
Yesterday I had the idea that we can allow suspending major release toasts for e.g. 8 weeks (=2 minor releases). But therefore we need to convert the GH release date string to a DateTime object. And I don't know how to do this in C++. - But this can be an improvement for later.

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Settings card is not disabled when version is != 0.0.1. Also info bar not displayed. @htcfreek can you double-check that?

@htcfreek
Copy link
Collaborator Author

htcfreek commented Feb 23, 2023

Settings card is not disabled when version is != 0.0.1. Also info bar not displayed. @htcfreek can you double-check that?

@stefansjfw

  • Settings card makes sense. My mistake we need a single & in the ViewModel property.
  • Are you running as normal user or do you debugging? (I know there is one case where the IsAdmin property is wrong. And btw, here we have the same mistake: We need a single &.

Edit: Bug found.

@stefansjfw
Copy link
Collaborator

Are you running as normal user or do you debugging?

Tested fully installed PT

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Looks good now :)

@htcfreek
Copy link
Collaborator Author

Then you can merge it. I don't have the permissions.

@@ -272,11 +274,20 @@ public bool IsAdmin
}
}

// Are we running a dev build? (Please note that we verify this in the code that gets the newest version from Github too.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

I'll update this one.

@jaimecbernardo jaimecbernardo merged commit 6750442 into microsoft:main Feb 24, 2023
@jaimecbernardo
Copy link
Collaborator

Thanks a lot for the contribution @htcfreek ! 🎉

@htcfreek htcfreek deleted the PT_UpdateGpo branch February 25, 2023 10:04
BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
* Implement GPO

* Add GPOs in updater

* Rename policy

* fix

* fix

* Update GPOWrapper.h

Added relative path to Generated Files folder for GPOWrapper.g.h

* fix and inactivate PeriodicUpdateCheck gpo

* Docs

* GPO name change

* Templates

* Templates: Text changes

* Templates: Text changes

* Templates: Text changes

* docs: spell fix

* settings ui

* fixes

* fixes

* fix gpo description

* EOF fix

* Fix include in UpdateUtils.cpp and remove build workaround

* UI improvements

* spell fixes

* code improvements

* Update README.md

* Update PowerToys.adml

* Update src/gpo/assets/PowerToys.admx

* Remove forbidden pattern
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Enterprise Issues relevant to large enterprises, SCCM, run as admin restrictions, ... Idea-Enhancement New feature or request on an existing product Needs-Review This Pull Request awaits the review of a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants