Add support for newer authentication API matching VS Code 1.63.1

[martin-fleck-at]

What it does

Fixes #9345
Closes #9498

• Stay compatible with newer and previous authentication API
  -- Keep prev authentication API matching VS Code 1.53.0-next.ea1b3f27db
  -- Add type files for newer authentication API and implement interfaces
  -- Bridge API version gap in plugin context and with merged types

• Add support for 'onAuthenticationRequest' activation event
  -- Allow dedicated trigger through 'ensureProvider' call
  -- Trigger activation if session of provider is requested

Note: This PR is based on the work of @PhilippeVienne done in this PR: #9498

How to test

I created two authentication provider plugins based on the sample app mentioned in the previous PR: https://github.com/martin-fleck-at/theia/tree/issue-9345-test

• One plugin is using the older VS Code API
• One plugin is using the newer VS Code API
• Provide commands to log in and log out of an account, we can see notifications and the proper Theia UI integration

• To test the onAuthenticationRequest activation, add github-authentication plugin (see logged message or notice how we actually are asked about the sign in)

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[msujew]

As far as I understood it, this PR supersedes #9498, right? Would you add a Closes #9498 to your PR in that case? This will automatically close the old PR once your's is merged.

[msujew]

@martin-fleck-at You mentioned two extensions (with old/new vscode extension API) in your testing steps, but only link to this one. Can you provide both extensions somewhere, so we can successfully run through all the testing steps?

[martin-fleck-at]

@msujew Absolutely, is it enough to simply add it to the description or do I need to add it in the commit message?

The link to the tests plugin is in the description but hidden among many others it seems ;-) https://github.com/martin-fleck-at/theia/tree/issue-9345-test. Specifically, the second commit adds the plugins (the first one being the PR): https://github.com/martin-fleck-at/theia/commit/ab73817ccc72eac8a89c2b73e3b109d5615defc2

[vince-fugnitto]

I'm not sure that I follow the practicality of the pull-request, perhaps you can clarify.

Given how the framework officially supports the API 1.53.2 (with a certain compatibility), how would we also support extensions that make use of the authentication api of 1.63. If a user were to bump the API in their product they would still be at a greater chance that the extension does not work for them given that all other 1.63.1 APIs are not implemented.

I'd probably be more fine if the changes were cleaner rather than versioning our files.

[vince-fugnitto]

Is this file really needed, or can we simply add it to theia.d.ts (is there a lot of incompatibilities between 1.53.2 and 1.63.0).

[martin-fleck-at]

@vince-fugnitto Thank you for your fast response!

The goal of this PR is to make Theia compatible with newer versions of the authentication API. The main reason being that both the authentication provider interface and the registration of an authentication provider has changed over time and make some plugins, such as the github-authentication plugin, unusable.

Maybe just to give a quick overview of the history:

• Theia - 2020: Implementation of the authentication API in Theia from API state from April 23, 2020 and some fixes later on.
• VSCode - Janurary 11, 2021: The authentication provider registration is revised.
• VSCode - February 3, 2021: Release of v1.53.0: https://github.com/microsoft/vscode/tree/1.53.0
• VSCode - February 8, 2021: Release of v1.53.1: https://github.com/microsoft/vscode/tree/1.53.1
• VSCode - February 11, 2021: Release of v1.53.2: https://github.com/microsoft/vscode/tree/1.53.2
• VSCode - February 11, 2021: Further changes were made to the authentication API: change event, scopes, method rename.
• VSCode - Feburary 23, 2021: API was moved from its proposed state to the stable VS Code API: microsoft/vscode@67ded11.
• VSCode - March 2, 2021: Release of v1.54.0 containing the stable API: https://github.com/microsoft/vscode/tree/1.54.0.
• VSCode - up to now: Additional options for session retrieval (forceNewSession, silent, clearSessionPreference) were added as optional fields to the API since then, not breaking any existing functionality afaik.

So, as it turns out we do not actually fully support the v1.53.2 since it would need to have the correct registration method but not the renamed methods, updated events or scope parameters. You can also observe this if you try the github-authentication plugin in version 1.53.2 which will not work, you would need to use a very specific commit, i.e., https://open-vsx.org/api/vscode/github-authentication/1.53.0-next.ea1b3f27db/file/vscode.github-authentication-1.53.0-next.ea1b3f27db.vsix.

Luckily, all those changes are made in a way that allows us to be backwards compatible. Specifically what we try to do in this commit is to bridge the difference between the APIs in the plugin context used by the plugins of the different versions. However, there are some changes (like changed types in the event) which need to be handled by merging the types and then querying the type, for instance in the sessions changed event in the authentication-main. In general, we aimed to make that change as least intrusive as possible. This is also the main reason, I moved the typings into a dedicated file instead of merging them into the Theia types (theia.d.ts), i.e., to make it obvious for the developer and to allow for easier transition if we ever were to add new versions, drop old ones, etc. Functionally, it does not make any difference at all so I can merge all types into one file if you prefer.

[vince-fugnitto]

@martin-fleck-at thank you for the details 👍

Luckily, all those changes are made in a way that allows us to be backwards compatible.

Given that the typings are backwards compatible (which should be the case in vscode since the API is finalized) I would merged them together with theia.d.ts and theia-proposed.d.ts respectfully. I'd avoid trying to version the files since it'll only get messier going forward, it might also be the case that we currently support newer APIs.

---

My main concern was that I was under the impression that in order to support newer extensions which are incompatible by definition (targeting a higher version than our default) was that it will cause issues on activation similarly to vscode (if it does not today I believe it might be a theia bug) - extensions from the vsx-registry view only install based on our default supported API :

In order to actually use an extension that targets a newer API an application developer would need to override the default value, or pass a value through the environment value, and as a consequence will start targetting a whole new set of extensions that are not compatible (going forward he might always pull, install, search extensions at 1.63 which is not implemented).

[martin-fleck-at]

@vince-fugnitto I followed your advice and merged the two type definitions. I still left the part of the old API that was previously in the proposed types in it's place but moved everything that is part of the official VS Code API to the normal Theia types.

As far as I know, Theia does not check whether the installed plugins fulfill a certain version. You are correct that in the OVSXClient we use the version to retrieve the latest compatible version but other than that no constraints are made. This allows us actually to simply install plugins via the theiaPlugins in the package.json. I would hope that this does not get removed and mostly results in a warning or something similar that indicates that the API might not be compatbile.

[vince-fugnitto]

As far as I know, Theia does not check whether the installed plugins fulfill a certain version.

@martin-fleck-at I believe we do, similarly to the following issue. I believe it might be a bug that we do not check the engines.vscode in this case and should fix it similarly to vscode. I don't think we want to arbitrarily allow any incompatible extension to be installed unless the application itself declares it supports it?

@msujew any thoughts?

[martin-fleck-at]

@vince-fugnitto I see what you mean. Yes, currently the engines.vscode does not get checked for sure. Still, I hope that if we fix that, we still make it configurable for adopters. I understand that it makes sense to set a default VS Code version that Theia is (mostly) compatible with to restrict the search in the VSX registry. And I see that this version can be configured. However, I can imagine a scenario where we want the Open VSX integration to work with a certain VS Code version (the one Theia is most compatible with) but still allow to add some additional plugins in the theiaPlugins section of the plugin.xml for a given product. For sure, the adopter has to take the responsibility to make sure the plugins work as expected but I think it is better than to absolutely forbid the installation.

[vince-fugnitto]

@martin-fleck-at right, by default we should align and do the check since it is the most basic verification we have not to pull extensions which have a greater chance of incompatibility with the current version of the framework. Downstream they can override this behavior or update the default API, but of course with the risk of using extensions which may be incompatible or use a different node runtime (for the most part they will want to stick with the framework).

If the changes work with 1.53.2 I'm fine with it, I was expecting however that the newer extension fails immediately due to engines.vscode so it's why I wanted to share my concern regarding the practicality of why we would support such a bump in API.

[msujew]

@vince-fugnitto We definitely don't check the engines.vscode version when installing/loading extensions, I would've been pretty annoyed by that by now while testing PRs :D

Fixing this behavior would probably be a good idea anyway, but we should probably make this configurable using a build-config entry or preference. I can imagine a lot of downstream-users accidentally using extensions with "invalid" engines.vscode fields which still run perfectly fine inside of Theia.

In general, as long as this PR does not break the existing "old" auth API but also adds support for the new API I'm completely fine with it. I will take a closer look at the changes later today 👍

[msujew]

@martin-fleck-at This is looking really good already. I have a few remarks about the code, but the functionality seems to work well already 👍

[msujew]

Minor: The nls.localizeByDefault function should be called for strings which already exist in vscode. This is a special case since confirmRelogin does not exist in our used version of nls.metadata.json, but that will be fixed once we bump the vscode API version.

Suggested change

	const message = recreatingSession
	? nls.localize('confirmRelogin', "The extension '{0}' wants you to sign in again using {1}.", extensionName, providerName)
	: nls.localize('confirmLogin', "The extension '{0}' wants to sign in using {1}.", extensionName, providerName);
	const message = recreatingSession
	? nls.localizeByDefault("The extension '{0}' wants you to sign in again using {1}.", extensionName, providerName)
	: nls.localizeByDefault("The extension '{0}' wants to sign in using {1}.", extensionName, providerName);

[martin-fleck-at]

Thank you very much for the explanation, I updated it as you suggested!

[msujew]

Minor: Please don't use constructor injection. Use field injection instead, see our coding guidelines.

[martin-fleck-at]

Ah yes, nice catch and sorry for the oversight.

[msujew]

Suggestion: We already have a timeout function in the promise-util which works quite similar. But instead of rejecting the promise, the timeout function resolves it. What do you think about adding a timeoutReject method which looks something like this?

export function timeoutReject(ms: number, message?: string): Promise<void> {
    const deferred = new Deferred<void>();
    setTimeout(() => deferred.reject(new Error(message)), ms);
    return deferred.promise;
}

This could also be used here.

I also thought about an additional override for timeout, but switching whether the promise resolves or rejects depending on an optional argument seems to me like a weird change of semantics.

Your opinion on that?

[martin-fleck-at]

I think that is a great idea! I was actually looking for a function like that but then simply decided to do it like in the vscode code. I took the method that you proposed since I am also not a huge fan of a Boolean parameter to switch between resolution and rejection since I know that I always have to double and triple check Boolean parameters when reading/writing to ensure correct usage ;-)

[martin-fleck-at]

@msujew Thank you very much for your thorough feedback! You were correct in all points and I adapted the code accordingly - I think it looks much cleaner now. I also adapted the test commit to have the old provider register without any of the new API to see if the delegation and everything still works as expected.

If you have any further comments, please let me know.

[msujew]

Great, the code is looking good for me, and I could not find any issues in the functionality of this change. I have a minor suggestion though 👍

• Old API continues to work correctly
• New API is working correctly
• Using both APIs is possible and works correctly as well

[martin-fleck-at]

@msujew Thank you very, very much for your continuous and thorough feedback, I really appreciate that! I fixed the last issue and squashed the commits so that they can be merged if the build runs through.

[msujew]

Thanks, with the recent changes everything is looking really good for me :)