DataBiosphere / leonardo

Notebook service
BSD 3-Clause "New" or "Revised" License
43 stars 21 forks source link

Ia 4997 third party cookies #4716

Closed mlilynolting closed 1 month ago

mlilynolting commented 1 month ago

Jira ticket: https://broadworkbench.atlassian.net/browse/IA-4997

Summary of changes

Can't test this locally or in a BEE, so here is a change which should be disabled everywhere by default.

NOTE: There are a bunch of nginx rewrites in the app helm charts here to add the Partitioned flag; for now I've commented them out and left the old logic in place. They ought not to be needed for me to test Galaxy under CHIPS.

Let's see what the Domain header does in Set-Cookie!

When the config flag IS_PROXY_COOKIE_PARTITIONED is set, Leo setCookie will respond with three Set-Cookie headers: one unscoped (will be blocked as a top-level third party cookie), one partitioned without a domain (will be scoped to the embedding domain, thus terra-ui), and one partitioned with a domain set to the proxy URL (might?? be scoped to the proxy domain and get past third-party protections on the new Galaxy tab!)

Testing these changes

We'll need to check Galaxy, Jupyter, Rstudio, and AoU tools with the flag set. This should be possible by pointing a local terra-ui at dev, with the proxy cookie flag set in leo's overrides.env file.

This is not intended as a final fix, just an attempt to understand whether the Domain attribute could possibly allow us to use CHIPS for now to get past our third-party cookie woes.

What to test

Who tested and where

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 73.91%. Comparing base (80bdc97) to head (06d499d). Report is 3 commits behind head on develop.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716/graphs/tree.svg?width=650&height=150&src=pr&token=NZGL9e1XHh&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere)](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere) ```diff @@ Coverage Diff @@ ## develop #4716 +/- ## =========================================== - Coverage 73.92% 73.91% -0.02% =========================================== Files 160 160 Lines 15000 15013 +13 Branches 1191 1228 +37 =========================================== + Hits 11089 11097 +8 - Misses 3911 3916 +5 ``` | [Files](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere) | Coverage Δ | | |---|---|---| | [...titute/dsde/workbench/leonardo/config/Config.scala](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?src=pr&el=tree&filepath=http%2Fsrc%2Fmain%2Fscala%2Forg%2Fbroadinstitute%2Fdsde%2Fworkbench%2Fleonardo%2Fconfig%2FConfig.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere#diff-aHR0cC9zcmMvbWFpbi9zY2FsYS9vcmcvYnJvYWRpbnN0aXR1dGUvZHNkZS93b3JrYmVuY2gvbGVvbmFyZG8vY29uZmlnL0NvbmZpZy5zY2FsYQ==) | `97.79% <100.00%> (+<0.01%)` | :arrow_up: | | [...e/dsde/workbench/leonardo/config/ProxyConfig.scala](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?src=pr&el=tree&filepath=http%2Fsrc%2Fmain%2Fscala%2Forg%2Fbroadinstitute%2Fdsde%2Fworkbench%2Fleonardo%2Fconfig%2FProxyConfig.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere#diff-aHR0cC9zcmMvbWFpbi9zY2FsYS9vcmcvYnJvYWRpbnN0aXR1dGUvZHNkZS93b3JrYmVuY2gvbGVvbmFyZG8vY29uZmlnL1Byb3h5Q29uZmlnLnNjYWxh) | `100.00% <ø> (ø)` | | | [...workbench/leonardo/util/BuildHelmChartValues.scala](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?src=pr&el=tree&filepath=http%2Fsrc%2Fmain%2Fscala%2Forg%2Fbroadinstitute%2Fdsde%2Fworkbench%2Fleonardo%2Futil%2FBuildHelmChartValues.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere#diff-aHR0cC9zcmMvbWFpbi9zY2FsYS9vcmcvYnJvYWRpbnN0aXR1dGUvZHNkZS93b3JrYmVuY2gvbGVvbmFyZG8vdXRpbC9CdWlsZEhlbG1DaGFydFZhbHVlcy5zY2FsYQ==) | `97.18% <ø> (ø)` | | | [...de/workbench/leonardo/http/api/CookieSupport.scala](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?src=pr&el=tree&filepath=http%2Fsrc%2Fmain%2Fscala%2Forg%2Fbroadinstitute%2Fdsde%2Fworkbench%2Fleonardo%2Fhttp%2Fapi%2FCookieSupport.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere#diff-aHR0cC9zcmMvbWFpbi9zY2FsYS9vcmcvYnJvYWRpbnN0aXR1dGUvZHNkZS93b3JrYmVuY2gvbGVvbmFyZG8vaHR0cC9hcGkvQ29va2llU3VwcG9ydC5zY2FsYQ==) | `56.52% <44.44%> (+2.67%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere). Last update [80bdc97...06d499d](https://app.codecov.io/gh/DataBiosphere/leonardo/pull/4716?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataBiosphere).
mlilynolting commented 1 month ago

@LizBaldo doesn't an unset flag evaluate to False? I thought that was the ? part of ${?FLAG}

LizBaldo commented 1 month ago

@LizBaldo doesn't an unset flag evaluate to False? I thought that was the ? part of ${?FLAG}

I am not sure actually, that's why I am thinking it might be safer to set it, but if you know otherwise feel free to go ahead

mlilynolting commented 1 month ago

No, the abundance of caution makes a ton of sense to me, thanks. I hadn't made a helmfile PR yet; just did: https://github.com/broadinstitute/terra-helmfile/pull/5741 (I had thought I'd be able to test it with the defaults but I was wrong)