-
Notifications
You must be signed in to change notification settings - Fork 9k
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
Added changes from @nicolashenry out of #9248 #9996
base: master
Are you sure you want to change the base?
Conversation
Sorry I wanted to finalize it but there are some breaking tests due to my change and I did not had time to fix those. I was due to the introduction of BroadcastChannel global not existing in a NodeJs environment used in the tests if I remember correctly. |
Any updates on this? |
any news here? |
@nicolashenry, do you know if https://www.npmjs.com/package/broadcast-channel would work to fix the broken tests? UPDATE: that is not a polyfill, so it's not a drop-in replacement so won't work. Luckily, it looks like https://www.npmjs.com/package/broadcastchannel-polyfill will work. I'm not sure if this is the cleanest fix, but this makes First, I ran diff --git a/test/unit/core/plugins/auth/actions.js b/test/unit/core/plugins/auth/actions.js
index d9c751be..62604056 100644
--- a/test/unit/core/plugins/auth/actions.js
+++ b/test/unit/core/plugins/auth/actions.js
@@ -1,3 +1,5 @@
+import { BroadcastChannel } from "broadcastchannel-polyfill"
+import { MessageChannel } from "node:worker_threads"
import { Map } from "immutable"
import win from "core/window"
import {
@@ -10,6 +12,9 @@ import {
} from "core/plugins/auth/actions"
import {authorizeAccessCodeWithBasicAuthentication, authPopup} from "../../../../../src/core/plugins/auth/actions"
+// broadcastchannel-polyfill expects MessageChannel to exist
+global.MessageChannel = MessageChannel
+
describe("auth plugin - actions", () => {
describe("authorizeRequest", () => { |
any updates? |
@ppena-LiveData should I add your change to this PR as well? |
@Cellebyte, if you think my suggested change is good, then yes, please add it to the PR so that the unit tests can pass. |
Description
This Pull Request should solve problems with
window.opener
not being available in some edge-cases when theoauth2-redirect.html
is used with an external auth provider.Motivation and Context
In the comments of #9248 there was a proper successor to solve the the original problem.
The functionality started breaking when browsers introduced
rel=noopener
fortarget=_blank
links.The reasons why it breaks in some setups is described in #8315.
closes #8030
closes #8315
How Has This Been Tested?
I did not test it yet but others did in #9248.
Checklist
My PR contains...
src/
is unmodified: changes to documentation, CI, metadata, etc.)package.json
)My changes...
Documentation
Automated tests