Allow disabling AuthenticationProvider extension for workspace if not yet activated#293130
Allow disabling AuthenticationProvider extension for workspace if not yet activated#293130gjsjohnmurray wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #269367 by allowing workspace-level disabling of authentication provider extensions that haven't been activated yet. Previously, all authentication provider extensions were blocked from workspace-level disablement to prevent issues with Settings Sync.
Changes:
- Added IExtensionService import to check extension activation status
- Modified
throwErrorIfCannotChangeWorkspaceEnablementto only prevent workspace-level disabling for auth provider extensions that are already activated - Made
instantiationServicea private readonly member to enable lazy service access
| throw new Error(localize('cannot disable auth extension in workspace', "Cannot change enablement of {0} extension in workspace because it contributes authentication providers", extension.manifest.displayName || extension.identifier.id)); | ||
| this.instantiationService.invokeFunction(accessor => { | ||
| const extensionService = accessor.get(IExtensionService); | ||
| if (extensionService.getExtensionsStatus()[extension.identifier.id]?.activationTimes) { |
There was a problem hiding this comment.
The check should also verify activationStarted in addition to activationTimes. Currently, the code only prevents disabling if the extension has fully activated (when activationTimes is populated). However, if activation has started but not yet completed (activationStarted: true, activationTimes: undefined), the extension could be in an intermediate state and shouldn't be disabled either. Consider checking: if (extensionService.getExtensionsStatus()[extension.identifier.id]?.activationStarted)
| if (extensionService.getExtensionsStatus()[extension.identifier.id]?.activationTimes) { | |
| const status = extensionService.getExtensionsStatus()[extension.identifier.id]; | |
| if (status?.activationStarted || status?.activationTimes) { |
| this.instantiationService.invokeFunction(accessor => { | ||
| const extensionService = accessor.get(IExtensionService); | ||
| if (extensionService.getExtensionsStatus()[extension.identifier.id]?.activationTimes) { | ||
| throw new Error(localize('cannot disable active auth extension in workspace', "Cannot change enablement of {0} extension in workspace because it contributes authentication providers and is already active", extension.manifest.displayName || extension.identifier.id)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The new logic allows workspace-level disabling of non-activated auth provider extensions, which changes existing behavior. The test at line 540 in extensionEnablementService.test.ts ('test canChangeWorkspaceEnablement return false for auth extension') will now fail because it expects all auth extensions to be non-changeable at workspace level, but with these changes, non-activated auth extensions should be changeable. New test coverage is needed to verify: 1) auth extensions that are not activated can be disabled at workspace level, and 2) auth extensions that are activated cannot be disabled at workspace level.
There was a problem hiding this comment.
I do not think we should access IExtensionService here. This caused cyclic dependency. The way you are using is a hack. Can you please explain why you need this?
There was a problem hiding this comment.
I want to discover whether the extension has already been activated. The extension service seemed to offer a way to do this. Initially I tried adding that service to the constructor but this flagged a cyclical dependency. So instead I am getting the service when I need it, which is a pattern I found in other places in the codebase.
If you don't think this is the right thing to do please suggest an alternative.
There was a problem hiding this comment.
Can you please remind me why we need to know if the extension is activated or not? Sorry that if it is mentioned already and me asking it again.
|
@gjsjohnmurray Probably the right fix here is to allow disabling extensions contributing auth providers which are not settings sync providers? |
|
@sandy081 that sounds good. If you give me a pointer I'm willing to try that strategy instead. However after Thursday I shall be OOO for 2 weeks. |
|
@gjsjohnmurray Here you go - |
This PR fixes #269367
If an extension contributing an AuthenticationProvider hasn't yet been activated then it is safe to disable for the current workspace.