-
Notifications
You must be signed in to change notification settings - Fork 22
Bug 2015833 - Fetch policies during engine initialization #459
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
Changes from all commits
01c3d4b
65b7aac
7ec51e1
d9c3981
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -102,7 +102,7 @@ EnterprisePoliciesManager.prototype = { | |||
| } | ||||
| }, | ||||
|
|
||||
| _initialize() { | ||||
| async _initialize() { | ||||
| this._cleanupPolicies(); | ||||
|
|
||||
| const changesHandler = provider => { | ||||
|
|
@@ -137,7 +137,7 @@ EnterprisePoliciesManager.prototype = { | |||
| this._status = Ci.nsIEnterprisePolicies.INACTIVE; | ||||
| Services.prefs.setBoolPref(PREF_POLICIES_APPLIED, false); | ||||
|
|
||||
| let provider = this._chooseProvider(changesHandler); | ||||
| let provider = await this._chooseProvider(changesHandler); | ||||
| if (provider.failed) { | ||||
| this._status = Ci.nsIEnterprisePolicies.FAILED; | ||||
| } | ||||
|
|
@@ -148,7 +148,7 @@ EnterprisePoliciesManager.prototype = { | |||
| Glean.policies.isEnterprise.set(this.isEnterprise); | ||||
| }, | ||||
|
|
||||
| _chooseProvider(handler) { | ||||
| async _chooseProvider(handler) { | ||||
| let platformProvider = null; | ||||
| if (AppConstants.platform == "win" && AppConstants.MOZ_SYSTEM_POLICIES) { | ||||
| platformProvider = new WindowsGPOPoliciesProvider(); | ||||
|
|
@@ -163,8 +163,13 @@ EnterprisePoliciesManager.prototype = { | |||
|
|
||||
| let jsonProvider = new JSONPoliciesProvider(); | ||||
| jsonProvider.onPoliciesChanges(handler); | ||||
|
|
||||
| let remoteProvider = RemotePoliciesProvider.createInstance(); | ||||
| // Fetch first set of remote policies during the | ||||
| // initialization of the policy engine | ||||
| await remoteProvider.fetchPoliciesOnStartup(); | ||||
| remoteProvider.onPoliciesChanges(handler); | ||||
|
|
||||
| if (platformProvider && platformProvider.hasPolicies) { | ||||
| if (jsonProvider.hasPolicies) { | ||||
| return new CombinedProvider( | ||||
|
|
@@ -415,14 +420,16 @@ EnterprisePoliciesManager.prototype = { | |||
| this.observersReceived.push(topic); | ||||
|
|
||||
| switch (topic) { | ||||
| case "policies-startup": | ||||
| // Before the first set of policy callbacks runs, we must | ||||
| // initialize the service. | ||||
| this._initialize(); | ||||
|
|
||||
| case "policies-startup": { | ||||
| const initializedPromise = this._initialize(); | ||||
| // _initialize() does async work (fetching remote policies). | ||||
| // We spin a nested event loop until the promise resolves so | ||||
| // this observer doesn't return before initialization completes. | ||||
| // This keeps startup behavior effectively synchronous. | ||||
| this.spinResolve(initializedPromise); | ||||
| this._runPoliciesCallbacks("onBeforeAddons"); | ||||
| break; | ||||
|
|
||||
| } | ||||
| case "profile-after-change": | ||||
| this._runPoliciesCallbacks("onProfileAfterChange"); | ||||
| break; | ||||
|
|
@@ -472,6 +479,44 @@ EnterprisePoliciesManager.prototype = { | |||
| } | ||||
| }, | ||||
|
|
||||
| /** | ||||
| * Spin the event loop until the passed promise resolves. | ||||
| * | ||||
| * This is used to await the response when fetching remote | ||||
| * policies during the initialization of the policy engine. | ||||
| * | ||||
| * @param {Promise} promise | ||||
| * @returns {any} Result of the resolved promise | ||||
| */ | ||||
| spinResolve(promise) { | ||||
| if (!(promise instanceof Promise)) { | ||||
| return promise; | ||||
| } | ||||
| let done = false; | ||||
| let result = null; | ||||
| let error = null; | ||||
| promise | ||||
| .catch(e => { | ||||
| error = e; | ||||
| }) | ||||
| .then(r => { | ||||
| result = r; | ||||
| done = true; | ||||
| }); | ||||
|
|
||||
| Services.tm.spinEventLoopUntil( | ||||
| "EnterprisePoliciesManager.sys.mjs:_initialize", | ||||
| () => done | ||||
| ); | ||||
| if (!done) { | ||||
| throw new Error("Forcefully exited event loop."); | ||||
| } else if (error) { | ||||
| throw error; | ||||
| } else { | ||||
| return result; | ||||
| } | ||||
| }, | ||||
|
|
||||
| messageDisallowedFeatures(neededOnContentProcess = false) { | ||||
| // NOTE: For optimization purposes, only features marked as needed | ||||
| // on content process will be passed onto the child processes. | ||||
|
|
@@ -954,6 +999,29 @@ class RemotePoliciesProvider { | |||
| this.triggerOnPoliciesChanges(); | ||||
| } | ||||
| } | ||||
|
|
||||
| async fetchPoliciesOnStartup() { | ||||
| if (!this._isPollingEnabled) { | ||||
| return; | ||||
| } | ||||
|
|
||||
| let res; | ||||
| try { | ||||
| res = await lazy.ConsoleClient.getRemotePolicies(); | ||||
| } catch (e) { | ||||
| console.error(`Failed to fetch remote policies on startup: ${e}`); | ||||
| this._failed = true; | ||||
| return; | ||||
| } | ||||
| if (!res.policies) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous can throw I think, so you want to catch too, not just nullcheck. |
||||
| console.error( | ||||
| `No policies were found in the response: ${JSON.stringify(res)}.` | ||||
| ); | ||||
| this._failed = true; | ||||
| } else { | ||||
| this._policies = res.policies; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this need to do a call/trigger/notify to explicitly process the policies?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yes. Good catch! I have to fetch the policies before calling enterprise-firefox/toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs Line 167 in 8c0694e
Let me verify, that that is correct and working.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified! |
||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| class WindowsGPOPoliciesProvider { | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming/guessing that the reason this uses a spinResolve instead of await is that
observeis non-async and defined inniIObserverso you can't change that, combined with needing_initializeto finish before calling_runPoliciesCallbacks. Is it worth adding a small comment here explaining the same?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some background is probably in the original PR #321 (comment)