feat: upgrade authentication API to latest vscode

[PhilippeVienne]

What it does

It implements the new authentication provider API from VSCode (the one moved from proposed to stable)

See #9345 for more details.

How to test

Use any vscode plugin depending on authentication like:
https://github.com/redhat-developer/vscode-redhat-account

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

[vince-fugnitto]

Please be sure to sign the Eclipse Contributor Agreement (ECA) with the same email as your authorship (currently fails one of the CI checks).

[vince-fugnitto]

@PhilippeVienne @theia/core should not depend on @theia/plugin, core is the parent-most extension of all extensions and should not depend on downstream extensions.

[PhilippeVienne]

So I may duplicate Interfaces here from the VSCode API ?

[vince-fugnitto]

I have not looked closely at the changes since I saw core was importing plugin. You should aim for a proper solution which does not couple extensions and does not introduce duplication.

[PhilippeVienne]

In fact there was previously a duplication, as we need to transport the same objects, I just corrected the code.

[PhilippeVienne]

Please be sure to sign the Eclipse Contributor Agreement (ECA) with the same email as your authorship (currently fails one of the CI checks).

It's an issue for me because my ECA account is on company email and not my comitter email

[vince-fugnitto]

It's an issue for me because my ECA account is on company email and not my comitter email

It is a hard requirement that the ECA is satisfied for all contributions (since this is an EclipseFoundation project). You can either update your authorship to your work email, or additionally sign the ECA with your committer email as well.

[PhilippeVienne]

It's an issue for me because my ECA account is on company email and not my comitter email

It is a hard requirement that the ECA is satisfied for all contributions (since this is an EclipseFoundation project). You can either update your authorship to your work email, or additionally sign the ECA with your committer email as well.

I signed with a new account

[PhilippeVienne]

@vince-fugnitto build has typing issue, I need more time fixing it

[PhilippeVienne]

@vince-fugnitto Normally this version is correct, you may contact me directly to have our own private extension for login (which will require little setup on your side to use it)

[vinokurig]

@PhilippeVienne what's the vscode version you are copying the code from?

[PhilippeVienne]

@PhilippeVienne what's the vscode version you are copying the code from?

I would say 1.54+ (but I taken it from main branch), have you a version requirement?

[vinokurig]

@PhilippeVienne Authentication API methods has been changed: https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/api/common/extHost.protocol.ts#L1201-L1208
https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/api/common/extHost.protocol.ts#L166-L173
could you please update them?

[PhilippeVienne]

@PhilippeVienne Authentication API methods has been changed: https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/api/common/extHost.protocol.ts#L1201-L1208
https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/api/common/extHost.protocol.ts#L166-L173
could you please update them?

@vinokurig I updated them

[vinokurig]

@PhilippeVienne I've tried the plugin you provided in the description, but I don't see the authentication menu

[clovis-dugue]

@vinokurig Sorry for the delay, our priorities shifted slightly in the last few weeks.

I am Philippe's coworker, and I brought together a sample app with an auth provider for you to test the PR easily with. I did not have any issue on my side, but be sure to tell me if something is wrong.

[vinokurig]

Provided example plugin works as expected. Thank you for the update!

Review is stale.

[PhilippeVienne]

@vinokurig @vince-fugnitto I have rebased the PR on latest branch to solve conflict

[PhilippeVienne]

@vinokurig an idea why test are failing only for Ubuntu, maybe rerun them?

[msujew]

@PhilippeVienne All Ubuntu CI runs are currently failing due to the monaco 0.23 upgrade, see #9702. You don't have to worry about that.

[msujew]

LGTM, the provided vscode extension seems to work well and the code looks good. I tried to test this with the GitHub extension as well, but that extension wouldn't start.

[msujew]

Nitpick: I'd personally prefer !!options?.supportsMultipleAccounts instead of options?.supportsMultipleAccounts ?? false

[PhilippeVienne]

@vinokurig @msujew can this PR be merged? (at least retry the failed build test)

[PhilippeVienne]

@vinokurig @msujew can you restart tests? I just rebase the branch on master

[PhilippeVienne]

Hello, all seems green, can this PR be merged ?

[vince-fugnitto]

@PhilippeVienne can you confirm if the changes are backwards compatible, to the API-level of 1.50.0 which is our current default? (I understand that you upgraded to the latest vscode).

[PhilippeVienne]

@vince-fugnitto it's true this is a 1.54+ move, as this, it will break the 1.50 retro-compat regarding authentication. I think it may be difficult but not impossible to ensure 1.50 retro-compatibility.

[vince-fugnitto]

@vince-fugnitto it's true this is a 1.54+ move, as this, it will break the 1.50 retro-compat regarding authentication. I think it may be difficult but not impossible to ensure 1.50 retro-compatibility.

@PhilippeVienne given that the changes will actually break 1.50.0 as you described, shouldn't we hold off on upgrading the authentication to when we support 1.54+ in the framework?

[vince-fugnitto]

As I mentioned in #9498 (comment), since the changes are not backwards compatible and target a version higher than our default supported API (now 1.53.2) we should hold off on the pull-request until we target 1.54.0.

[martin-fleck-at]

I am also very interested in this PR to get the latest version of the github-authentication plugin a little bit closer to working properly.

@PhilippeVienne Thank you very much for your work, this is exactly the update I was looking for!

@vince-fugnitto Are there any plans to update the default supported VS Code API or is there anything else we can do to get this PR merged?

LGTM, the provided vscode extension seems to work well and the code looks good. I tried to test this with the GitHub extension as well, but that extension wouldn't start.

@msujew This might be due to the fact that we only activate the plugin currently if someone calls $ensureProvider. In VS Code they try to activate any authentication provider as soon as someone retrieves or creates a session with a provider given a certain id, see https://github.com/microsoft/vscode/blob/37e8ec1fc02fdec890dc088cbf3b0703bb87dd4e/src/vs/workbench/services/authentication/browser/authenticationService.ts#L699. So that might be something that should be added here as well.

I'd gladly take a look at this again if nobody else has time but only if there is any chance to get that merged somehow.

[vince-fugnitto]

@vince-fugnitto Are there any plans to update the default supported VS Code API or is there anything else we can do to get this PR merged?

@martin-fleck-at I don't believe we have any formal criteria for bumping the API but we generally did so when we were satisfied with the API compatibility coverage we achieved for a specific version. At the moment I believe we might still have too many APIs still missing, but I may be wrong (ex: the delta from 1.53.2 and 1.54.0 might be minimal)

In addition, we would probably want to update electron (which we recently did) and possibly update monaco (which I believe is planned) before we can think about bumping the API (like we've done in the past).

As it stands I believe the pull-request targets newer versions of the API but does not make the default we support backwards compatible which was the main reason for not merging.

[martin-fleck-at]

@vince-fugnitto Thank you for letting me know! I'll try to make a version of that change that keeps backwards compatibility and post the link here as soon as I have a version ready.

@PhilippeVienne I'll need to open a new PR for this since I cannot push on top of your branch. I hope this is fine with you. If you have any concerns or would like to take on the work yourself, please let me know. Thank you again for the great work!