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

Apply vscode Authentication plugin API #8402

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Apply vscode Authentication plugin API #8402

merged 1 commit into from
Sep 9, 2020

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Aug 18, 2020

What it does

Apply vscode Authentication plugin API: https://github.com/microsoft/vscode/blob/ab42ffc44c709aded523b15399a070ae8c724824/src/vs/vscode.proposed.d.ts#L98-L131

Related CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22526

How to test

  1. Download the sample plugin or clone and compile the sample project: https://github.com/vinokurig/authentication.git
  2. Start the hosted plugin
  3. Click the Accounts icon in the left bottom side and see:
    screenshot-localhost_3000-2020 08 31-11_55_24
  4. Run F1 => Authentication Sign in command and click Allow:
    screenshot-localhost_3000-2020 08 31-11_57_08
    see the token message:
    screenshot-localhost_3030-2020 08 31-12_06_46
  5. Sign out from the account with the help of the account menu:
    screenshot-localhost_3030-2020 08 31-12_08_22
    screenshot-localhost_3030-2020 08 31-12_10_03
    screenshot-localhost_3000-2020 08 31-12_26_19
  6. Click the Accounts icon and see the login item:
    screenshot-localhost_3000-2020 08 31-12_27_57
    Click the login item and the token notification must appear:
    screenshot-localhost_3030-2020 08 31-12_06_46
  7. Run F1 => Authentication Sign in command and see:
    screenshot-localhost_3000-2020 08 31-12_52_31

vscode has manage trusted extensions feature that was not implemented because it requires quick-pick with multi-select: #5673

Review checklist

Reminder for reviewers

@vinokurig vinokurig added plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) labels Aug 18, 2020
@vinokurig vinokurig requested a review from akosyakov August 18, 2020 14:50
@akosyakov akosyakov added the security issues related to security label Aug 24, 2020
@akosyakov akosyakov requested a review from benoitf August 24, 2020 12:07
@akosyakov
Copy link
Member

akosyakov commented Aug 24, 2020

I don't have time to review it. @azatsarynnyy @tsmaeder @benoitf Could someone to take it over?

Important concerns here:

  • security: probably extension should no easy grab any session, but there should be some actions from users to grant permissions. We should check with VS Code how it works and enforced.
  • extensibility: When we integrate authentication within other products there should be a way to register providers from Theia extensions, i.e. imagine user token in Gitpod (it does not come from VS Code extension). Probably there is something like that in Che too.

@akosyakov
Copy link
Member

#8372 brings abilities to add menus at the bottom of the left side bar. In VS Code there are commands to manage accounts, as well as menu items including in the left side bar. Should not this PR cover it as well?

@vinokurig
Copy link
Contributor Author

vinokurig commented Aug 26, 2020

@akosyakov
We can add those actions in the next iteration as an improvements after the new menu items PR is merged.

@vinokurig
Copy link
Contributor Author

#8372 is merged so I'll try to port the authentication actions from vscode in this PR

@azatsarynnyy
Copy link
Member

Thanks for providing the test plugin!

I'm testing it using the provided plugin and I see an empty row in the quick pick at step 5:
image

After choosing it nothing happens.

@akosyakov
Copy link
Member

#8372 is merged so I'll try to port the authentication actions from vscode in this PR

@vinokurig I am fine with another PR too if this PR is complete in some sense.

Could you elaborate please?

  • How it is ensured that an arbitrary VS Code extension cannot access an arbitrary token?
  • How one can add a custom provider via Theia extension, i.e. if my product already does authentication with GitHub and I would like to provide a token to VS Code extensions.

Rather than that please rely on @azatsarynnyy for the approve.

packages/core/src/browser/authentication-service.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/authentication-ext.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/authentication-ext.ts Outdated Show resolved Hide resolved
packages/plugin/src/theia.d.ts Show resolved Hide resolved
@vinokurig
Copy link
Contributor Author

@azatsarynnyy I've updated the test plugin and the how to test section` according to the new left bottom menu actions, so now the quick pick should show correct items.

@vinokurig
Copy link
Contributor Author

@tsmaeder I've fixed all your comments, could you please review the PR again?

@azatsarynnyy
Copy link
Member

azatsarynnyy commented Sep 1, 2020

It seems something wrong with Sign out dialog.
image

It's different in the provided How to test section.

Other steps work well.

@vinokurig
Copy link
Contributor Author

@azatsarynnyy Thank you for finding this bug, looks like my browser have cached this data. I was able to reproduce it in incognito mode. I've fixed it in my last commit.

@vinokurig
Copy link
Contributor Author

@tsmaeder Could you please review this PR again?

@vinokurig
Copy link
Contributor Author

@tsmaeder @azatsarynnyy I've fixed all your comments, could you please review this PR again?

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

LGTM

@vinokurig
Copy link
Contributor Author

@tsmaeder any updates?

@tsmaeder tsmaeder dismissed their stale review September 8, 2020 07:13

All comments have been addressed, but I can't approve, since I didn't test the code.

@vinokurig
Copy link
Contributor Author

@akosyakov @westbury is it OK to merge?

@akosyakov
Copy link
Member

yes, please clean up the history before

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) plug-in system issues related to the plug-in system security issues related to security vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants