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

♻️ Renaming session cookie once again #6544

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Oct 16, 2024

What do these changes do?

Apparently Firefox handles cookies differently than Chrome and Safari. It keeps cookies with the same name but different domains.
Even after invalidating the session key for the cookies. You login into Firefox open a new style dynamic service and authentication fails. Apparently it send out old cookie, the one with domain osparc.io (which is invalid) instead of the one with .osparc.io domain. This causes a 401 reply when opening UUID.services.osparc.io.

To avoid issues for users, the session cookie is being renamed.

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK marked this pull request as ready for review October 16, 2024 09:00
Copy link

@GitHK GitHK added this to the MartinKippenberger milestone Oct 16, 2024
@GitHK GitHK self-assigned this Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.1%. Comparing base (cafbf96) to head (bf40ca7).
Report is 638 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6544      +/-   ##
=========================================
+ Coverage    84.5%   88.1%    +3.5%     
=========================================
  Files          10    1548    +1538     
  Lines         214   63350   +63136     
  Branches       25    2059    +2034     
=========================================
+ Hits          181   55836   +55655     
- Misses         23    7195    +7172     
- Partials       10     319     +309     
Flag Coverage Δ
integrationtests 64.7% <ø> (?)
unittests 86.1% <100.0%> (+1.5%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ings-library/src/settings_library/utils_session.py 44.4% <100.0%> (ø)

... and 1497 files with indirect coverage changes

@YuryHrytsuk
Copy link
Contributor

YuryHrytsuk commented Oct 16, 2024

This will break a few e2e ops tests because we explicitly rely on Cookie name. We need to update these tests as soon as this fix is rolled out.

Furthermore, this needs to be done in stages (master --> stag --> prod)

I will take care of it

@YuryHrytsuk
Copy link
Contributor

YuryHrytsuk commented Oct 16, 2024

This will break a few e2e ops tests because we explicitly rely on Cookie name. We need to update these tests as soon as this fix is rolled out.

Furthermore, this needs to be done in stages (master --> stag --> prod)

I will take care of it

I wonder if there are other places that explicitly rely on this cookie name. I hope there are no such 3rd parties

@GitHK
Copy link
Contributor Author

GitHK commented Oct 16, 2024

This will break a few e2e ops tests because we explicitly rely on Cookie name. We need to update these tests as soon as this fix is rolled out.
Furthermore, this needs to be done in stages (master --> stag --> prod)
I will take care of it

I wonder if there are other places that explicitly rely on this cookie name. I hope there are no such 3rd parties

inside the simcore codebase there are no other places. And we have tests to avoid cookie being out of sync that fail.
regarding other repos I would not know

@GitHK GitHK merged commit 55c967d into ITISFoundation:master Oct 16, 2024
57 checks passed
@GitHK GitHK deleted the pr-osparc-rename-cookie-for-ffx branch October 16, 2024 10:08
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.

4 participants