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

♻️ Is3318/refactoring websever.login plugin (2/3) #3590

Merged
merged 68 commits into from
Dec 8, 2022

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 20, 2022

What do these changes do?

Follows from #3577 and is mostly a refactoring of the login plugin and some preparations to send 2FA codes via email.

  • ♻️ refactoring of login plugin
    • Cleanup login plugin's settings (moved all messages to login._constants
    • Using pg transactions for atomic delete (to avoid inconsistent states)
    • Enhanced error handling
      • OECs when unexpected errors (e.g. emails failures)
      • for failures on email confirmation links, redirection to front-end error-page
      • request validation errors transformed from ValidationError to 422
    • Removes dependencies from openapi-core:
      • adds routes=RouteTableDef() decorators to login handlers.
      • replaces extract_and_validate with servicelib.aiohttp.requests_validation and pydantic models which in addition are used to create the OAS
  • ♻️ moved most test_login to services/web/server/tests/unit/with_dbs/03/login, cleaned and extended.
  • ♻️ refactoring of email plugin setup
  • web-server API 0.12.0 → 0.12.2
  • ✨ Adds services/web/server/src/simcore_service_webserver/templates/common/new_2fa_code.jinja2 email template

Next PR will implement

  • new entrypoint to resend 2FA code (via email or SMS)
  • restrict access to login_2fa, register_phone and phone_confirmation handlers (via temporary access codes in session cookies)

Further improvements (subsequent PRs)

  • Increase Password requirements #2480
  • session cookie expiration + secured
  • geo location of ips
  • GC task to clean old CONFIRMATION_PENDING users and invalid CONFIRMATION?
  • move auto_add_user_to_groups to email_confirmation when the latter is required.
  • username (as identity)+ multiple emails?

Related issue/s

How to test

cd services/web/server
make install-dev
pytest  tests/unit/with_dbs/03/login

Checklist

  • rm all MSG_ constants from config
  • Use models from pydantic validation of requests
  • unique user-name _get_user_name
  • check auto_add_user_to_groups with @sanderegg
  • Atomic transactions to avoid inconsistent states of data
  • make version-*
  • make openapi.json
  • cd packages/postgres-database, make setup-commit, sc-pg review -m "my changes"
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@pcrespov pcrespov self-assigned this Nov 20, 2022
@pcrespov pcrespov added the a:webserver issue related to the webserver service label Nov 20, 2022
@pcrespov pcrespov added this to the Athena milestone Nov 20, 2022
@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

Merging #3590 (c08c261) into master (0f04727) will decrease coverage by 2.3%.
The diff coverage is 87.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3590     +/-   ##
========================================
- Coverage    82.3%   79.9%   -2.4%     
========================================
  Files         763     880    +117     
  Lines       33745   37179   +3434     
  Branches      778     785      +7     
========================================
+ Hits        27792   29728   +1936     
- Misses       5746    7243   +1497     
- Partials      207     208      +1     
Flag Coverage Δ
integrationtests 62.7% <57.9%> (+15.5%) ⬆️
unittests 77.5% <87.6%> (-3.8%) ⬇️

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

Impacted Files Coverage Δ
...server/src/simcore_service_webserver/_constants.py 100.0% <ø> (ø)
...server/src/simcore_service_webserver/login/_sql.py 96.0% <ø> (ø)
...rver/src/simcore_service_webserver/login/routes.py 100.0% <ø> (ø)
...er/src/simcore_service_webserver/login/settings.py 96.5% <ø> (-3.5%) ⬇️
...c/simcore_service_webserver/login/_registration.py 84.7% <40.0%> (+1.3%) ⬆️
.../web/server/src/simcore_service_webserver/email.py 92.8% <50.0%> (+6.4%) ⬆️
...ver/src/simcore_service_webserver/utils_aiohttp.py 81.3% <58.3%> (-12.6%) ⬇️
...ver/src/simcore_service_webserver/login/storage.py 94.2% <63.6%> (-5.8%) ⬇️
...rary/src/servicelib/aiohttp/requests_validation.py 92.6% <66.6%> (-4.8%) ⬇️
...src/simcore_service_webserver/login/utils_email.py 67.0% <66.6%> (+1.2%) ⬆️
... and 216 more

@pcrespov pcrespov changed the title ✨ Is3318/resend sms handler (2/2) WIP: ✨ Is3318/resend sms handler (2/2) Nov 20, 2022
@pcrespov pcrespov force-pushed the is3318/resend-sms-handler branch 3 times, most recently from e86c61d to 51a6df3 Compare November 25, 2022 12:25
@pcrespov pcrespov force-pushed the is3318/resend-sms-handler branch 5 times, most recently from 06a7d85 to 9db9205 Compare December 6, 2022 14:16
@pcrespov pcrespov changed the title WIP: ✨ Is3318/resend sms handler (2/2) WIP: ✨ Is3318/resend sms handler (2/3) Dec 6, 2022
@pcrespov pcrespov force-pushed the is3318/resend-sms-handler branch 5 times, most recently from de74f9a to defa89d Compare December 7, 2022 22:50
@pcrespov pcrespov changed the title WIP: ✨ Is3318/resend sms handler (2/3) WIP: ♻️ Is3318/resend sms handler (2/3) Dec 7, 2022
@pcrespov pcrespov changed the title WIP: ♻️ Is3318/resend sms handler (2/3) ♻️ Is3318/refactoring websever.login plugin (2/3) Dec 7, 2022
@pcrespov pcrespov marked this pull request as ready for review December 7, 2022 23:58
@pcrespov pcrespov requested review from odeimaiz and mguidon December 7, 2022 23:58
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.

great! and thanks for moving the tests to their own folder

Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

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

👌

@pcrespov pcrespov force-pushed the is3318/resend-sms-handler branch from ee69f86 to bd01628 Compare December 8, 2022 10:33
@odeimaiz odeimaiz self-requested a review December 8, 2022 13:27
@pcrespov pcrespov force-pushed the is3318/resend-sms-handler branch from dbe7f61 to 81fbd9f Compare December 8, 2022 13:30
@pcrespov pcrespov enabled auto-merge (squash) December 8, 2022 14:57
@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@pcrespov pcrespov merged commit 5d22707 into ITISFoundation:master Dec 8, 2022
@pcrespov pcrespov deleted the is3318/resend-sms-handler branch December 8, 2022 17:12
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.

3 participants