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

🐛 Fixes auth product error in vendor services 🚨 #6512

Merged
merged 25 commits into from
Oct 14, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 10, 2024

What do these changes do?

The Forward auth Traefik middleware, introduced in this pull request, uses the webserver's GET v0/auth:check endpoint to authenticate vendor services, whether deployed dynamically (e.g., a Jupyter service started in a study) or statically (e.g., the S4L manual).

Forward Auth Middleware

The GET v0/auth:check endpoint relies on the products middleware to determine which product is being requested. It then checks if the user is authenticated and authorized for that product. Since the Forward auth middleware handles the request, we needed to use the X-Forward-Host header to identify the originating hostname instead of relying solely on the hostname itself.

We also tried sending the X-Simcore-Product header from the front-end when loading the site in an iframe, but we ran into CORS issues. For now, we've abandoned this approach.

Key Highlights

  • 🐛 product middleware
    • Now detects the product from the X-Forwarded-Host header when used in the middleware.
  • ♻️ session plugin
    • Separated the session._cookie_storage module and added some robustness improvements.
  • ♻️ security plugin
    • Introduced a constant for the product.login permission key.

IMPORTANT: This approach requires further testing, but we've committed it to unblock first the master-e2e deployments. Following PRs will address this #6522

Related issue/s

How to test

cd services/web/server
make install-dev
pytest -v tests/unit/isolated/test_products_middlewares.py

Manually

make up-prod
make show-endpoints

image

  • open main site (1) -> shows login page
  • open Vendor Manual (Fake) (2) -> no access
  • login in (1)
  • open (2) -> access. shows request info (it a https://github.com/traefik/whoami service )
  • logout in (1)
  • open (2) -> no access

Dev-ops checklist

None

@pcrespov pcrespov added the a:webserver issue related to the webserver service label Oct 10, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Oct 10, 2024
@pcrespov pcrespov self-assigned this Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 89.65517% with 6 lines in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
...mcore_service_webserver/session/_cookie_storage.py 80.6% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6512      +/-   ##
=========================================
+ Coverage    84.5%   88.1%    +3.5%     
=========================================
  Files          10    1548    +1538     
  Lines         214   63346   +63132     
  Branches       25    2059    +2034     
=========================================
+ Hits          181   55822   +55641     
- Misses         23    7207    +7184     
- Partials       10     317     +307     
Flag Coverage Δ
integrationtests 64.7% <81.0%> (?)
unittests 86.1% <89.6%> (+1.5%) ⬆️

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

Files with missing lines Coverage Δ
.../simcore_service_webserver/application_settings.py 99.4% <ø> (ø)
.../src/simcore_service_webserver/login/decorators.py 100.0% <100.0%> (ø)
...ver/src/simcore_service_webserver/products/_api.py 73.8% <ø> (ø)
...simcore_service_webserver/products/_middlewares.py 100.0% <100.0%> (ø)
...imcore_service_webserver/security/_authz_policy.py 95.2% <100.0%> (ø)
...c/simcore_service_webserver/security/_constants.py 100.0% <100.0%> (ø)
...rver/src/simcore_service_webserver/security/api.py 100.0% <100.0%> (ø)
...er/src/simcore_service_webserver/session/errors.py 100.0% <100.0%> (ø)
...er/src/simcore_service_webserver/session/plugin.py 100.0% <100.0%> (ø)
...src/simcore_service_webserver/statics/_handlers.py 50.0% <ø> (ø)
... and 1 more

... and 1487 files with indirect coverage changes

@pcrespov pcrespov force-pushed the is6506/fix-auth-product branch from 0f1e4ad to 157cc6d Compare October 11, 2024 14:33
@pcrespov pcrespov changed the title WIP: 🐛 Is6506/fix auth product 🐛 Fixes auth product error in vendor services 🚨 Oct 11, 2024
@pcrespov pcrespov marked this pull request as ready for review October 11, 2024 14:33
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.

Thanks!

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

There are 2 things that will not work here.
Let's discuss about it.

@pcrespov pcrespov requested a review from GitHK October 14, 2024 08:13
Copy link

Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

very nice PR I didnt catch anything it looks very reaosnable! thanks

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Very good thanks for the changes.

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

pair reviewed. thanks!

@pcrespov pcrespov merged commit da15add into ITISFoundation:master Oct 14, 2024
57 checks passed
@pcrespov pcrespov deleted the is6506/fix-auth-product branch October 14, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication failing when only s4l product is present
6 participants