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

fix(oauth2): add fallback for case when window.opener is null #9248

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

notpushkin
Copy link

@notpushkin notpushkin commented Sep 25, 2023

Motivation and Context

Sometimes window.opener might be null (there's multiple issues about that). Currently this results in a blank page without any indication of what might have gone wrong.

Related: #8315, #3227
Fixes #8030, fixes #6150

Description

This PR adds a fallback: if window.opener is null, it will display a one-liner to run on a Swagger UI page (using devtools) in a prompt(). Not too elegant, but given this is an edge case I'm not sure if it warrants more complex UI.

How Has This Been Tested?

I've checked the added snippet independently by running it in devtools and it works as intended.

I'll be grateful for pointers to how to cover it with unit / integrations tests :-)

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@nicolashenry
Copy link

I think that window.postMessage could be used to communicate with the original opener instead of doing it manually (by using the oauth2 state to check the origin).

Copy link

@ftsell ftsell left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer of swagger-ui but I found your PR and can generally confirm it to be working apart from two minor bugs

dev-helpers/oauth2-redirect.html Outdated Show resolved Hide resolved
dev-helpers/oauth2-redirect.html Outdated Show resolved Hide resolved
@ftsell
Copy link

ftsell commented Jan 13, 2024

I think that window.postMessage could be used to communicate with the original opener instead of doing it manually (by using the oauth2 state to check the origin).

@nicolashenry Could you elaborate? I'm interested in getting this fixed and am willing to contribute.

@notpushkin
Copy link
Author

@nicolashenry This is how it usually works already, yeah. The problem is, when window.opener is null we can't use postMessage, so we need some kind of a fallback mechanism for that – this is what I'm trying to achieve here.

@ftsell Thank you so much for the review! Fixed both issues.

@ftsell
Copy link

ftsell commented Jan 14, 2024

I looked into using postMessage() for this usecase and while I agree that it is probably the better way for cross-tab communication in general the postMessage() function is also located on a window object. This means that it also requires a reference to the opener which is precisely what we're missing :/

An alternative I found is to use a BroadcastChannel but that would broadcast authentication related information through the whole browser which is also not optimal.

@nicolashenry
Copy link

Yes, I was thinking about BroadcastChannel.postMessage and not Window.postMessage. Sorry for the mistake.
Creating a unique identifier (a "nonce" for example) in the oauth "state" should be enough to differentiate which window is the origin of the call.

@notpushkin
Copy link
Author

@nicolashenry Sounds like a good idea. Wanna try implement it?

@nicolashenry
Copy link

@notpushkin I made this quickly : 940ebe0, I have not tested it yet but I will try to turn this into a PR this week if it works.

@donkee
Copy link

donkee commented Feb 2, 2024

@nicolashenry Have you tested/made a PR for your fix?

@markus-ki
Copy link

Are there plans to include the fix in a release? I ran into the same problem and this solution from @nicolashenry works fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

window.opener is null in oauth2-redirect.html oauth2-redirect and email links
5 participants