Fix not sending necessary HTTP headers when downloading results #41633

WiNloSt · 2024-04-19T11:17:54Z

Related to #18823

Description

This fix the problem where users logged in via Google SSO couldn't download question results. This was because we call fetch directly without passing the proper CSRF header which is required when we authenticate with metabase.EMBEDDED_SESSION cookie like Google SSO in this case. That is why we got 401 in the response.

How to verify

Follow the repro steps in Google SSO with Interactive Embedding do not allow you to download CSV #39848
Ensure Fix download full results link for embedded and public questions is still working

To set up subpath. Use https://github.com/WiNloSt/metabase-subpath. The instruction is already in the repo.

Checklist

Tests have been added/updated to cover changes in this PR

replay-io · 2024-04-19T11:29:24Z

Status	Complete ↗︎
Commit	`0e52c6c`
Results	⚠️ 12 Flaky ✅ 2418 Passed

WiNloSt · 2024-04-19T13:12:34Z

e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js

+      const CSRF_TOKEN = "abcdefgh";
+      cy.intercept("GET", "/api/user/current", req => {
+        req.on("response", res => {
+          res.headers["X-Metabase-Anti-CSRF-Token"] = CSRF_TOKEN;


I just mock the header as it's virtually impossible to set up Google SSO at the moment.

WiNloSt · 2024-04-19T13:13:19Z

e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js

+
+      cy.wait("@CsvDownload").then(interception => {
+        expect(
+          interception.request.headers["x-metabase-anti-csrf-token"],


Then I only assert that we pass the correct header. As downloaded files are tested in other tests.

WiNloSt · 2024-04-19T13:14:08Z

frontend/src/metabase/lib/api.js

+              body = options.transformResponse({
+                body,
+                data,
+                response: unreadResponse,


Since I need to pass a new key, I think it's a good time to refactor transformResponse API, although nobody uses this API outside this PR.

WiNloSt · 2024-04-19T13:14:32Z

frontend/src/metabase/query_builder/actions/downloading.ts

@@ -42,6 +43,12 @@ export const downloadQueryResults =
    }
  };

+const downloadChart = async ({ question }: DownloadQueryResultsOpts) => {


Swap function locations because downloadChart is used first.

WiNloSt · 2024-04-19T13:17:47Z

frontend/src/metabase/query_builder/actions/downloading.ts

@@ -168,9 +173,20 @@ const getDatasetResponse = ({
        formattedBody.append(key, JSON.stringify(body[key]));
      }
    }
-    return fetch(requestUrl, { method, body: formattedBody });
+    return POST(requestUrl, {


Use POST and GET from frontend/src/metabase/lib/api.js as it takes care of the passing CSRF token.

I was considering a more static approach where I define the path upfront. But the path is not always static since we also call

metabase/frontend/src/metabase/query_builder/actions/downloading.ts

Line 102 in a14999f

url: Urls.publicQuestion({ uuid, type, includeSiteUrl: false }),

WiNloSt · 2024-04-19T15:25:12Z

frontend/src/metabase/query_builder/actions/downloading.ts

-  const requestUrl = new URL(url, location.origin);
-  return requestUrl.href;
+
+  return url;


We need to ensure we now only return the relative path without the subpath. Previously we needed the whole path because we passed it directly to fetch but this PR passed it to GET and POST which already take care of the subpath via api.basename.

i.e. if we deploy metabase on /metabase and the download URl is /metabase/embed/question/123.csv. This function needs to return just /embed/question/123.csv as you'll see in the unit test below

WiNloSt · 2024-04-19T15:26:02Z

frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx

-          );
-        }),
-      ).toBe(true);
+      const [url, options] = mockDownloadEndpoint.lastCall() as MockCall;


Since we changed to use GET and POST. options.body turns out to be a Promise instead. I'm not totally sure why.

WiNloSt · 2024-04-22T08:26:23Z

@uladzimirdev I requested your review since you last touch this file with this bug fix #34659

npretto

Code looks good to me

* Fix not sending necessary HTTP headers when downloading results * Fix fail E2E tests * Add E2E test * Fix failed tests * Fix unit tests * Fix download on subpath * Fix unit tests for subpath

… results" (#41682) * Fix not sending necessary HTTP headers when downloading results (#41633) * Fix not sending necessary HTTP headers when downloading results * Fix fail E2E tests * Add E2E test * Fix failed tests * Fix unit tests * Fix download on subpath * Fix unit tests for subpath * Fix `api.js`'s `transformResponse` API --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Mahatthana Nomsawadi <mahatthana.n@gmail.com>

* Add a way to update filter type and operator from parameter sidebar (#41555) * Convert EmbeddingContext to Sdk Store slice (#41585) * Add prefix to CSS custom properties (#41574) * Migrate --border-* * Migrate *breadcrumb* * Migrate --color-bg-black * Migrate --color-bg-dark * Migrate --color-bg-light * Migrate --color-bg-medium * Migrate --color-bg-white * Migrate --color-border * Migrate --color-brand* * Migrate --color-error * Migrate --color-focus * Migrate --color-shadow * Migrate --color-success * Migrate --color-text-dark * Migrate --color-text-light * Migrate --color-text-medium * Migrate --color-text-white * Migrate --color-white * Migrate --default-border-radius * Migrate --default-button-border-radius * Migrate --default-font-family * Migrate --default-font-size * Migrate --default-header-margin * Migrate --gap-1 * Migrate --icon-width * Migrate --input-border-color * Migrate --input-border-radius * Migrate --margin-* * Migrate --muted-color * Migrate --padding-* * Migrate --page-header-padding * Migrate --subtitle-color * Migrate --title-color * Reset to master * Remove unused --color-text-default * Remove unused --color* * Remove unused CSS custom properties * Prefix only necessary CSS custom properties * Fix Dispatch type (#41631) * Fix Dispatch type * Fix typing * Fix types * [Feature branch] Split data access and query builder access (#41581) Co-authored-by: Noah Moss <noahbmoss@gmail.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Nick Fitzpatrick <nick@metabase.com> Co-authored-by: John Swanson <john.swanson@metabase.com> Co-authored-by: Sloan Sparger <sloansparger@users.noreply.github.com> Co-authored-by: Sloan Sparger <sloansparger@gmail.com> * Re-wire the `Groups` entity to use RTK Query under the hood (#41480) * Delete unused endpoint from `PermissionsApi` * Add group tag helpers * Define RTK Query 5 main endpoints for `permissionApi` (group) * Re-wire 5 main api methods in groups entity to work with RTK Query * Re-wire `clearMember` action to work with RTK Query * Add `Groups.api.update` * Update types * Fix membership failing to update after `CREATE_MEMBERSHIP` * Make sure `members` is not undefined * Fix cache invalidation * Use dbname from database details instead of stats returned from Mongo (#41549) * Use dbname from details instead of from stats * Add mongo sharded cluster CI job * add withUndos option to renderWithProviders to render Undos during … (#41576) * Account for non-hydrated properties in GET /api/dashboard (#41622) * Account for non-hydrated properties in GET /api/dashboard * Fix provideTags --------- Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com> * [docs] Revise Okta SAML docs (#41493) * okta docs * clarifying group mapping methods * links * Apply suggestions from code review Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com> * review feedback * Typo --------- Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com> * Remove defaultauthdb portion of mongo connection string (#41553) * Reset mapping after change of operator for native questions (#41615) * Reset mapping after change of operator for native questions * Add a basic test * Better name for a test * Add a new migration to clean up deprecated perm types (#41649) * [QP, lib] Add new expression functions for host, domain, subdomain (#41540) These functions are implemented with hairy regular expressions, and it's more user-friendly and future-proof to name those functions in MBQL rather than baking the `regexextract` and regex into the user's query. It lets us evolve the regexes in the future if we detect a bug, and it improves the UX since the user sees a meaningful function instead of regexextract([My URL Column], "(?<=[@\.])(?!www\.)[^@\.]+(?=\.[^@\.]{1,3}\.[^@\.]+$|\.[^@\.]+$)") Also refactors the regexes somewhat so that they work for emails as well as URLs, and there's always just one layer of `:regex-match-first`. Previously this was separated into two steps: URL or email to host, and host to (sub)domain. Part of the follow-up for Extract Column epic #38964. * New default DB permissions (#40869) * Re-wire the `Revisions` entity to use RTK Query under the hood (#41558) * Add RTK Query `revision` API * Re-wire `Revisions` entity `list` to work with RTK Query * Re-wire the `Revisions` entity `revert` to work with RTK Query * Rename 'mac' alias to 'macaw' (#41680) * Fix not sending necessary HTTP headers when downloading results (#41633) * Fix not sending necessary HTTP headers when downloading results * Fix fail E2E tests * Add E2E test * Fix failed tests * Fix unit tests * Fix download on subpath * Fix unit tests for subpath * Re-wire the `Snippets` entity to use RTK Query under the hood (#41656) * Throw for `Snippets.api.delete` * Add boilerplate `snippet` RTK API * Add boilerplate RTK-compatibility layer for main `Snippets.api` endpoints * Add types * Add cache invalidation * add validated migration checksums 45 46 47 and 48 (#41662) * Modify JWT session token logic to use immediate return value from action (#41694) * Port six collection-related components to TypeScript (#41632) BaseItemsTable BaseTableItem BulkActions CollectionContentView CollectionCopyEntityModal ItemsTable * [tech debt] Stop using ms/BooleanString and only use ms/BooleanValue (#41390) * [tech debt] Stop using ms/BooleanString and only use ms/BooleanValue ms/BooleanString will coerce to a string, and we should probably get rid of it and only ever use ms/BooleanValue which will always coerce to a boolean We want the coercion layer to give us values that are helpful, and true is more helpful than "true". So we should remove ms/BooleanString and calls to parseBoolean * Remove ms/BooleanString usage in api/native_query_snippet.clj * Remove test case related to ms/BooleanString * Remove the BooleanString schema def as no longer used * Dates Binned by Week Export Formatting Matches App (#41619) * Dates Binned by Week Export Formatting Matches App Fixes: #41492 Before, the dates binned by week would be formatted as follows in CSV and JSON exports: `Week 1 - 2024` But that doesn't match the App's format of a Date Range: `January 1, 2024 - January 7, 2024` So, now the exports will apply formatting in this same way. Note that a second bug (#41616) exists on the Frontend preventing the column formatting being applied in the app. This is frontend only, and the csv and json exports will match the column formatting settings (eg. abbreviated dates and alternative separators will be used in the export). * add test and change default week style * Week format test change * also fix incorrect :hour formatting * New default permissions for groups (#41323) * Add dynamic fonts to embedding SDK (#41179) * Fix TippyPopover default text color not visible after migration (#41716) * Use the proper case for a "year" token format (#41715) * Use proper case for year date format Fixes #40493 * Add regression test for specific filters in different locales * Actually use the locale * Note the format_rows changes in the api changelog (#41268) * Improve test coverage for describe-fks and describe-fields (#41528) * Exclude internal content from admin checklist and usage stats (#41697) * Replace `isLoggedIn` and `isInitialized` with `LoginStatus` (#41637) * move cache business logic to model from api (#41677) * Migrate Entity Copy Modal away from EntityForm (#41534) * CopyQuestionForm * CopyDashboardForm * inital values adjustment, e2e tests * FormObserver * Remove unused `monospaceText` prop (#41721) * Remove unused function (#41720) * Be more consistent with borders and avoid double borders (#41565) * Be more consistent with borders and avoid double borders * Mark the aggregation 'Custom Expression' button as an action * Remove color from action headers * Remove bottom border from last child * Remove top border from first child * Make section header classes consistent * Add border to search header * Use correct color for borders * Add props to enable borders in AccordionList * Add borders to column picker elements * dont use buildjet for e2e or frontend tests (#41733) * dont use buildjet for e2e * dont use buildjet for frontend either --------- Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com> Co-authored-by: Oisin Coveney <oisin@metabase.com> Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Noah Moss <noahbmoss@gmail.com> Co-authored-by: Nick Fitzpatrick <nick@metabase.com> Co-authored-by: John Swanson <john.swanson@metabase.com> Co-authored-by: Sloan Sparger <sloansparger@users.noreply.github.com> Co-authored-by: Sloan Sparger <sloansparger@gmail.com> Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com> Co-authored-by: lbrdnk <lbrdnk@users.noreply.github.com> Co-authored-by: Nicolò Pretto <info@npretto.com> Co-authored-by: Alex Yarosh <alexandra@metabase.com> Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com> Co-authored-by: Braden Shepherdson <braden@metabase.com> Co-authored-by: Tim Macdonald <tim@metabase.com> Co-authored-by: bryan <bryan.maass@gmail.com> Co-authored-by: Raphael Krut-Landau <raphael.kl@gmail.com> Co-authored-by: A. Marius Rabenarivo <mariusrabenarivo@gmail.com> Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com> Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com> Co-authored-by: Alexander Solovyov <alexander@solovyov.net> Co-authored-by: Romeo Van Snick <romeo@romeovansnick.be> Co-authored-by: Ryan Laurie <30528226+iethree@users.noreply.github.com>

Fix not sending necessary HTTP headers when downloading results

94067b9

metabase-bot bot assigned WiNloSt Apr 19, 2024

metabase-bot bot added the .Team/Embedding label Apr 19, 2024

Fix fail E2E tests

4447a78

WiNloSt added the backport Automatically create PR on current release branch on merge label Apr 19, 2024

WiNloSt added 2 commits April 19, 2024 19:55

Add E2E test

d6ec129

Fix failed tests

a14999f

WiNloSt commented Apr 19, 2024

View reviewed changes

WiNloSt requested a review from a team April 19, 2024 13:22

WiNloSt added 3 commits April 19, 2024 21:50

Fix unit tests

5f4397f

Fix download on subpath

c406d8a

Fix unit tests for subpath

0e52c6c

WiNloSt commented Apr 19, 2024

View reviewed changes

WiNloSt requested a review from uladzimirdev April 22, 2024 08:25

npretto approved these changes Apr 22, 2024

View reviewed changes

WiNloSt merged commit 82d90d0 into master Apr 22, 2024
109 checks passed

WiNloSt deleted the 39848-cannot-download-csv-in-google-sso-interactive-embedding branch April 22, 2024 11:30

metabase-bot bot mentioned this pull request Apr 22, 2024

🤖 backported "Fix not sending necessary HTTP headers when downloading results" #41682

Merged

WiNloSt added this to the 0.49.7 milestone Apr 22, 2024

WiNloSt mentioned this pull request Apr 23, 2024

Remove unused function #41720

Merged

1 task

WiNloSt removed this from the 0.49.7 milestone Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix not sending necessary HTTP headers when downloading results #41633

Fix not sending necessary HTTP headers when downloading results #41633

WiNloSt commented Apr 19, 2024 •

edited

replay-io bot commented Apr 19, 2024 •

edited

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt Apr 19, 2024

WiNloSt commented Apr 22, 2024

npretto left a comment

Fix not sending necessary HTTP headers when downloading results #41633

Fix not sending necessary HTTP headers when downloading results #41633

Conversation

WiNloSt commented Apr 19, 2024 • edited

Description

How to verify

Checklist

replay-io bot commented Apr 19, 2024 • edited

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt Apr 19, 2024

Choose a reason for hiding this comment

WiNloSt commented Apr 22, 2024

npretto left a comment

Choose a reason for hiding this comment

WiNloSt commented Apr 19, 2024 •

edited

replay-io bot commented Apr 19, 2024 •

edited