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 refs with authentication being broken if the fetch for iframe.html succeeds (but with a request to authenticate) #18160

Merged
merged 5 commits into from
Aug 2, 2022

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented May 6, 2022

Issue:
I discovered that when a ref is defined (could be an auto-ref) which requires authentication; storybook will (server-side) check to see if it's a public storybook.

It checks this because public storybook do not need authentication headers to be sent (which require more server/cors-setup).

The idea is that if NodeJS can make a request to the iframe.html resource and get a statusCode 200 that means the storybook can be referenced without authentication headers at runtime in the web-app.

So what happened?
The node side of storybook makes a request to the iframe.html and the service returns a HTML page with a login form; with statusCode 200.
Arguably the service should return a non-200 statusCode, but it doesn't..

What I did

I added an additional check to re-request the resource with an accept-header, and check the absence of a loginUrl field on the JSON response.

If the response is not valid JSON this additional check is ignored, and the previously behavior is preserved.

How to test

  • Add a ref to an example that requires the user to authenticate
  • Test if the composed storybook is loaded correctly
  • Test if the authentication flow works.

…esource, we'll get a 200 response only when the resource was accessible.

But in actuallity it's normal for a 200 statusCode to be returned but it's actually an authentication page.

I added an additional check to re-request the resource with an accept-header, and check the absence of a loginUrl field on the JSON response.

If the response is not valid JSON this additional check is ignored, and the previously behavior is preserved.
@nx-cloud
Copy link

nx-cloud bot commented May 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 382fe39. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen self-assigned this May 6, 2022
@ndelangen ndelangen requested a review from tmeasday May 6, 2022 12:34
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Qn

Then do an additional check with JSON headers to check for tier2 auth-check
@ndelangen ndelangen requested a review from shilman May 11, 2022 09:43
@ndelangen ndelangen requested a review from tmeasday July 2, 2022 15:52
…atic

# Conflicts:
#	lib/manager-webpack4/src/manager-config.ts
#	lib/manager-webpack5/src/manager-config.ts
@ndelangen ndelangen merged commit 5de2b21 into next Aug 2, 2022
@ndelangen ndelangen deleted the fix/refs-with-authentication-hosted-on-chromatic branch August 2, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants