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

plugin-ext: attach security cookie to webviews #7847

Merged
merged 1 commit into from
May 26, 2020

Conversation

paul-marechal
Copy link
Member

When on Electron, we use a cookie to tell if an http client can use the
backend services. Cookies are bound to domains to which requests will be
made, and also subdomains in certain situations. Because we are using
localhost the cookie cannot be shared across subdomains. This commit
works around this limitation by passing the security token from
electron-main process to the renderer-process via a global variable,
and the token is then set inside a cookie for each new webview endpoint
created by the plugin system in the frontend.

How to test

Install and use https://github.com/vince-fugnitto/webview-example/releases/download/1.0.0/cat-coding-0.0.1.vsix, it should work on Electron.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added bug bugs found in the application electron issues related to the electron target webviews issues related to webviews labels May 19, 2020
@paul-marechal
Copy link
Member Author

The localhost limitation regarding setting cookies for subdomains was really not intuitive... But the issue boils down to some domain property that must be set in the cookie, but that also must be at least two parts: something like a.b which is not the case for the localhost domain (only one part).

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified that webviews now successfully work on electron 🍾

Screen Shot 2020-05-19 at 3 43 04 PM

@paul-marechal
Copy link
Member Author

@benoitf @akosyakov refactored the widget factory into new inversify modules.

@akosyakov
Copy link
Member

Please merge if it works

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

Please look at comments before merging. I don’t think entry points designed well, they cannot be reused and separate code which can only work together. I think they should be removed there is no real motivation for reuse right now.

@paul-marechal
Copy link
Member Author

@benoitf is this change to the inversify containers ok with you? cc @akosyakov

@@ -50,7 +50,8 @@
{
Copy link
Member

@akosyakov akosyakov May 26, 2020

Choose a reason for hiding this comment

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

You can add frontendElectron in another entry point to rebind something from frontend of first entry point:

"theiaExtensions": [
    {
      "backend": "lib/plugin-ext-backend-module",
      "backendElectron": "lib/plugin-ext-backend-electron-module",
      "frontend": "lib/plugin-ext-frontend-module"
    },
    {
      "frontendElectron": "lib/plugin-ext-frontend-electron-module"
    }
  ]

It would be the minimal change.

@marcdumais-work
Copy link
Contributor

Let me give this a quick test

When on Electron, we use a cookie to tell if an http client can use the
backend services. Cookies are bound to domains to which requests will be
made, and also subdomains in certain situations. Because we are using
`localhost` the cookie cannot be shared across subdomains. This commit
works around this limitation by passing the security token from
electron-main process to the renderer-process via a global variable,
and the token is then set inside a cookie for each new webview endpoint
created by the plugin system in the frontend.

Signed-off-by: Paul Maréchal <[email protected]>
@marcdumais-work
Copy link
Contributor

It works! 👍

There are notifications that appear every few seconds. The message is not very specific :)

Peek 2020-05-26 16-51

@paul-marechal
Copy link
Member Author

Will wait on CI to be green again and merge then.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented May 26, 2020

@marechal-p did you see the notifications popping-up when you tested?

@paul-marechal
Copy link
Member Author

@marcdumais-work didn't notice that on your screen it is missing the icons for some reason. On windows I have them displayed correctly. The notifications are part of the cat coding VS Code extension.

@marcdumais-work
Copy link
Contributor

I see it on the browser version as well, so prob unrelated to this PR

@marcdumais-work
Copy link
Contributor

@marcdumais-work didn't notice that on your screen it is missing the icons for some reason. On windows I have them displayed correctly. The notifications are part of the cat coding VS Code extension.

Strange. You're right, browser version has them:
image

@paul-marechal
Copy link
Member Author

Opened a followup issue based on your issue with icons @marcdumais-work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application electron issues related to the electron target webviews issues related to webviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants