🐛 Is812/email case insensitive #3953

pcrespov · 2023-03-07T16:29:31Z

What do these changes do?

🐛 These changes make emails' case insensitive.
- Now we will store all emails in the database in lower case, and when a user wants to authenticate with upper case, or wants to add user to the group with upper case (as is mentioned in the issue below) it will be possible.

Related issue/s

Closes e-mails in simcore should be case-insensitives osparc-issues#812

How to test

Checklist

codecov · 2023-03-07T16:43:16Z

Codecov Report

Merging #3953 (add93f3) into master (6032512) will increase coverage by 1.2%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master   #3953     +/-   ##
========================================
+ Coverage    85.0%   86.2%   +1.2%     
========================================
  Files         935     797    -138     
  Lines       40313   35578   -4735     
  Branches      848     394    -454     
========================================
- Hits        34286   30687   -3599     
+ Misses       5804    4782   -1022     
+ Partials      223     109    -114

Flag	Coverage Δ
integrationtests	`66.5% <87.5%> (-0.1%)`	⬇️
unittests	`83.1% <100.0%> (+0.8%)`	⬆️

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

Impacted Files	Coverage Δ
.../simcore_service_catalog/db/repositories/groups.py	`59.4% <100.0%> (ø)`
...server/src/simcore_service_webserver/groups_api.py	`97.0% <100.0%> (+0.5%)`	⬆️
.../src/simcore_service_webserver/invitations_core.py	`80.0% <100.0%> (ø)`
...r/src/simcore_service_webserver/login/_security.py	`100.0% <100.0%> (ø)`
...c/simcore_service_webserver/login/handlers_auth.py	`94.1% <100.0%> (ø)`
...ver/src/simcore_service_webserver/login/storage.py	`94.5% <100.0%> (+0.3%)`	⬆️

... and 144 files with indirect coverage changes

…espov/osparc-simcore into is812/email-case-insensitive

sanderegg · 2023-03-13T10:31:56Z

Very nice addition, check please if test_groups.py changes really make sense?

matusdrobuliak66 · 2023-03-13T10:43:45Z

Very nice addition, check please if test_groups.py changes really make sense?

I tried to test 3 scenarios:

adding the user to the group as it was till now
adding the user to the group with the uppercase email even though we stored him in the database with lowercase (this is testing the situation of issue e-mails in simcore should be case-insensitives osparc-issues#812)
adding the user to the group with the lowercase email even though he was registered with the uppercase email (testing that the registration worked and it was changed to the lowercase so we can compare correctly)

sanderegg

it seems my comments did not make it through this morning. really sorry about this. now this should work. I have a few questions regarding test_groups.py syntax. we can also talk about it tomorrow.

services/web/server/tests/unit/with_dbs/01/test_groups.py

services/web/server/tests/unit/with_dbs/03/login/test_login_auth.py

codeclimate · 2023-03-14T07:09:29Z

Code Climate has analyzed commit add93f3 and detected 0 issues on this pull request.

View more on Code Climate.

sonarcloud · 2023-03-14T07:09:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

GitHK

While you changes work. I am worried that in the future we might stumble upon bugs due to the different way we treat emails. We check agains the db for email [email protected] but in a template we might have [email protected] and this could cease some unexpected issues.
My proposal is to apply lower case as soon as the email hits one of the entry points of a service and always use it as lower case.

GitHK · 2023-03-14T07:03:27Z

services/catalog/src/simcore_service_catalog/db/repositories/groups.py

@@ -39,7 +39,9 @@ async def get_user_gid_from_email(
    ) -> Optional[PositiveInt]:
        async with self.db_engine.connect() as conn:
            return await conn.scalar(
-                sa.select([users.c.primary_gid]).where(users.c.email == user_email)
+                sa.select([users.c.primary_gid]).where(
+                    users.c.email == user_email.lower()


This is not what I would have expected. I would have changed the email at the point of ingress in the application. At the edge (on the ingress API route).
Doing so gives the guarantee that the email string is the same during the entire request and not treated differently in some cases.
I would track where this is coming from and change it accordingly at that entry point.

GitHK · 2023-03-14T07:04:21Z

services/web/server/src/simcore_service_webserver/groups_api.py

+        result = await conn.execute(
+            sa.select([users]).where(users.c.email == email.lower())
+        )


Same comment as above, please check the source(s) and apply lower case at the closes point of entry in the application.

GitHK · 2023-03-14T07:05:37Z

services/web/server/src/simcore_service_webserver/login/_security.py

@@ -24,7 +24,7 @@ async def login_granted_response(

    Uses security API
    """
-    email = user["email"]
+    email = user["email"].lower()


GitHK · 2023-03-14T07:07:27Z

services/web/server/src/simcore_service_webserver/login/handlers_auth.py

@@ -110,7 +110,7 @@ async def login(request: web.Request):
        )

    assert user["status"] == ACTIVE, "db corrupted. Invalid status"  # nosec
-    assert user["email"] == login_.email, "db corrupted. Invalid email"  # nosec
+    assert user["email"] == login_.email.lower(), "db corrupted. Invalid email"  # nosec


And here as well I would make sure that login_.email is already lower case when used here

GitHK · 2023-03-14T07:07:52Z

services/web/server/src/simcore_service_webserver/login/storage.py

+            if with_data.get("email"):
+                with_data["email"] = with_data["email"].lower()


GitHK · 2023-03-14T07:08:00Z

services/web/server/src/simcore_service_webserver/login/storage.py

+            if data.get("email"):
+                data["email"] = data["email"].lower()


and here once more

pcrespov added 2 commits March 7, 2023 17:28

drafted test

24a80cb

dc

d1e3d24

pcrespov assigned pcrespov and matusdrobuliak66 Mar 7, 2023

pcrespov added a:webserver issue related to the webserver service changelog:🐛bugfix labels Mar 7, 2023

pcrespov added this to the Mithril milestone Mar 7, 2023

matusdrobuliak66 and others added 4 commits March 9, 2023 08:04

making user emails lowercase

d1da7b4

drafted test

179733b

dc

0229556

making user emails lowercase

6819f9c

pcrespov force-pushed the is812/email-case-insensitive branch from d1da7b4 to 6819f9c Compare March 9, 2023 11:47

pcrespov and others added 9 commits March 9, 2023 17:24

minor cleanup

7e6ee4e

fixes invitation email check

3223db0

mionr

47ef8ee

Merge branch 'master' into is812/email-case-insensitive

626435e

Merge branch 'is812/email-case-insensitive' of https://github.com/pcr…

463d106

…espov/osparc-simcore into is812/email-case-insensitive

adding login auth email check

71bd3dd

Merge branch 'master' into is812/email-case-insensitive

89b74de

cleanup of unit tests

3fa8adb

Merge branch 'master' into is812/email-case-insensitive

12ddbe7

matusdrobuliak66 marked this pull request as ready for review March 13, 2023 10:07

matusdrobuliak66 requested review from sanderegg and GitHK as code owners March 13, 2023 10:07

matusdrobuliak66 changed the title ~~WIP: 🐛 Is812/email case insensitive~~ 🐛 Is812/email case insensitive Mar 13, 2023

matusdrobuliak66 added 2 commits March 13, 2023 11:15

cleanup of unit tests

57536dc

Merge branch 'is812/email-case-insensitive' of https://github.com/pcr…

745f89c

…espov/osparc-simcore into is812/email-case-insensitive

sanderegg reviewed Mar 13, 2023

View reviewed changes

matusdrobuliak66 and others added 2 commits March 14, 2023 08:03

fix & refactor of unit tests after code review

f02717e

Merge branch 'master' into is812/email-case-insensitive

add93f3

GitHK reviewed Mar 14, 2023

View reviewed changes

matusdrobuliak66 closed this Mar 20, 2023

matusdrobuliak66 mentioned this pull request Mar 20, 2023

🐛 Is812/email case insensitive v2 (⚠️ devops) 🚨 #3998

Merged

pcrespov deleted the is812/email-case-insensitive branch March 29, 2023 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Is812/email case insensitive #3953

🐛 Is812/email case insensitive #3953

pcrespov commented Mar 7, 2023 •

edited by matusdrobuliak66

Loading

codecov bot commented Mar 7, 2023 •

edited

Loading

sanderegg commented Mar 13, 2023

matusdrobuliak66 commented Mar 13, 2023 •

edited

Loading

sanderegg left a comment

codeclimate bot commented Mar 14, 2023

sonarcloud bot commented Mar 14, 2023

GitHK left a comment

GitHK Mar 14, 2023

GitHK Mar 14, 2023

GitHK Mar 14, 2023

GitHK Mar 14, 2023

GitHK Mar 14, 2023

GitHK Mar 14, 2023

		if with_data.get("email"):
		with_data["email"] = with_data["email"].lower()

🐛 Is812/email case insensitive #3953

🐛 Is812/email case insensitive #3953

Conversation

pcrespov commented Mar 7, 2023 • edited by matusdrobuliak66 Loading

What do these changes do?

Related issue/s

How to test

Checklist

codecov bot commented Mar 7, 2023 • edited Loading

Codecov Report

sanderegg commented Mar 13, 2023

matusdrobuliak66 commented Mar 13, 2023 • edited Loading

sanderegg left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 14, 2023

sonarcloud bot commented Mar 14, 2023

GitHK left a comment

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

GitHK Mar 14, 2023

Choose a reason for hiding this comment

pcrespov commented Mar 7, 2023 •

edited by matusdrobuliak66

Loading

codecov bot commented Mar 7, 2023 •

edited

Loading

matusdrobuliak66 commented Mar 13, 2023 •

edited

Loading