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

🎨 Adds authentication for new style dynamic services and platform vendor services ⚠️ #6484

Merged

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Oct 2, 2024

⚠️ devops

What do these changes do?

Traefik's forwardauth middleware is used to authenticate requests based on cookies. This allows for services under the same subdomain as osparc.io to no longer be shown if the user is not logged in.

  • 🎨 extended webserver to share cookies with all it's subdomains to enable cookie authentication across subdomains (from my research this is safe since there is no way for a malicious actor to overwrite the cookie's domain)
  • ✨ added /v0/auth:check to webserver which allows Traefik's middleware to check authentication of incoming requests with negligible impact on performance
  • 🎨 new style dynamic services require login
  • ✨ added vendor services stack
  • ✨ added manual service inside vendor stack with possibility to configure image domain and replicas (for turning on and off)

Side effects for new style dynamic services

After a service is opened in a browser, users could typically copy the UUID.services.osparc.io address and open it somewhere else. Under the following conditions this is no longer possible:

  • a new "private" window is opened
  • opened in a different browser
  • if the portal was opened on Safari in Private Browsing mode all tabs in the window do not share any information (normal mode still works)

Side effect login behaviour change

This has no impact on any of our deployments.

Some tests were running on http://127.0.0.1:9081. This no longer works, the cookie is not set because .127.0.0.1 is not a valid domain name for the cookie.
Instead use http://127.0.0.1.nip.io:9081 which sets the cookie's domain to .127.0.0.1.nip.io which is a valid domain.

Related issue/s

How to test

Dev-ops checklist

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
...er/src/simcore_service_webserver/session/plugin.py 80.0% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6484      +/-   ##
=========================================
+ Coverage    84.5%   88.1%    +3.5%     
=========================================
  Files          10    1540    +1530     
  Lines         214   63138   +62924     
  Branches       25    2058    +2033     
=========================================
+ Hits          181   55647   +55466     
- Misses         23    7176    +7153     
- Partials       10     315     +305     
Flag Coverage Δ
integrationtests 64.7% <80.0%> (?)
unittests 86.1% <83.3%> (+1.5%) ⬆️

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

Files with missing lines Coverage Δ
...ctor_v2/core/dynamic_services_settings/__init__.py 100.0% <100.0%> (ø)
...ules/dynamic_sidecar/docker_service_specs/proxy.py 100.0% <100.0%> (ø)
.../simcore_service_webserver/login/_auth_handlers.py 97.3% <100.0%> (ø)
...er/src/simcore_service_webserver/session/plugin.py 87.8% <80.0%> (ø)

... and 1486 files with indirect coverage changes

@GitHK GitHK added this to the MartinKippenberger milestone Oct 2, 2024
@GitHK GitHK self-assigned this Oct 2, 2024
@GitHK GitHK changed the title 🎨 Extend authentication based on cookies to new style dynamic services and add support for user manual for logged in users 🎨 Extended authentication to new style dynamic services and platform vendor services Oct 2, 2024
@GitHK GitHK changed the title 🎨 Extended authentication to new style dynamic services and platform vendor services 🎨 Adds authentication for new style dynamic services and platform vendor services Oct 2, 2024
@GitHK GitHK marked this pull request as ready for review October 4, 2024 13:54
@GitHK GitHK changed the title 🎨 Adds authentication for new style dynamic services and platform vendor services 🎨 Adds authentication for new style dynamic services and platform vendor services ⚠️ Oct 4, 2024
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Thx. Looks real good. I left some suggestions

Andrei Neagu added 3 commits October 8, 2024 08:37
Copy link

sonarcloud bot commented Oct 8, 2024

@GitHK GitHK enabled auto-merge (squash) October 8, 2024 06:39
Copy link
Contributor

@jsaq007 jsaq007 left a comment

Choose a reason for hiding this comment

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

👍🏼

@GitHK GitHK merged commit fbc6446 into ITISFoundation:master Oct 8, 2024
57 checks passed
@GitHK GitHK deleted the pr-osparc-manual-for-logged-in-users branch October 8, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dynamic-sidecar dynamic-sidecar service a:webserver issue related to the webserver service security Pull requests that address a security vulnerability t:enhancement Improvement or request on an existing feature t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants