From 5a38c477e2c81400c343955806af023ed1e3fc75 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 1 Apr 2025 13:52:07 +0200 Subject: [PATCH 1/9] Show 'Change password' and 'Edit profile' only when user created account through our registration form --- server/mergin/auth/api.yaml | 2 ++ server/mergin/auth/models.py | 5 +++++ server/mergin/auth/schemas.py | 1 + web-app/packages/lib/src/modules/user/types.ts | 1 + .../lib/src/modules/user/views/ProfileViewTemplate.vue | 5 ++++- 5 files changed, 13 insertions(+), 1 deletion(-) diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index 095b0d26..ba51967e 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -895,6 +895,8 @@ components: example: my-workspace role: $ref: "#/components/schemas/WorkspaceRole" + can_edit_profile: + type: boolean LoginResponse: allOf: - $ref: "#/components/schemas/UserDetail" diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 31499ad3..8577070e 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -236,6 +236,11 @@ def create( db.session.commit() return user + @property + def can_edit_profile(self) -> bool: + """Flag if we allow user to edit their email and name""" + return self.passwd is not None + class UserProfile(db.Model): user_id = db.Column( diff --git a/server/mergin/auth/schemas.py b/server/mergin/auth/schemas.py index 62d0bd1e..3f614ae1 100644 --- a/server/mergin/auth/schemas.py +++ b/server/mergin/auth/schemas.py @@ -101,6 +101,7 @@ class UserInfoSchema(ma.SQLAlchemyAutoSchema): receive_notifications = fields.Boolean(attribute="profile.receive_notifications") registration_date = DateTimeWithZ(attribute="registration_date") name = fields.Function(lambda obj: obj.profile.name()) + can_edit_profile = fields.Boolean(attribute="can_edit_profile") class Meta: model = User diff --git a/web-app/packages/lib/src/modules/user/types.ts b/web-app/packages/lib/src/modules/user/types.ts index 2305c053..bb1f0b29 100644 --- a/web-app/packages/lib/src/modules/user/types.ts +++ b/web-app/packages/lib/src/modules/user/types.ts @@ -66,6 +66,7 @@ export interface UserDetailResponse extends UserProfileResponse { receive_notifications: boolean registration_date: string workspaces: UserWorkspace[] + can_edit_profile: boolean } export interface WorkspaceResponse extends UserWorkspace { diff --git a/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue b/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue index 4c6632a3..7326f055 100644 --- a/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue +++ b/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue @@ -12,7 +12,10 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial >

Account details

-
+
Date: Tue, 1 Apr 2025 14:17:33 +0200 Subject: [PATCH 2/9] Backend blocks update profile for SSO users --- server/mergin/auth/api.yaml | 8 ++++++-- server/mergin/auth/controller.py | 9 +++++++++ server/mergin/auth/models.py | 1 + 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index ba51967e..b42b2628 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -68,8 +68,8 @@ paths: post: tags: - user - summary: Update profile of user in sesssion - description: Update profile of user in sesssion + summary: Update profile of user in session + description: Update profile of user in session operationId: mergin.auth.controller.update_user_profile requestBody: description: Updated profile @@ -101,6 +101,8 @@ paths: $ref: "#/components/responses/BadStatusResp" "401": $ref: "#/components/responses/UnauthorizedError" + "403": + $ref: "#/components/responses/Forbidden" /app/auth/refresh/csrf: get: summary: Get refreshed csrf token @@ -426,6 +428,8 @@ paths: description: OK "400": $ref: "#/components/responses/BadStatusResp" + "403": + $ref: "#/components/responses/Forbidden" "404": $ref: "#/components/responses/NotFoundResp" /app/auth/reset-password/{token}: diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 980bc14c..69ba2009 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -255,6 +255,8 @@ def logout(): # pylint: disable=W0613,W0612 @auth_required def change_password(): # pylint: disable=W0613,W0612 + if not current_user.can_edit_profile: + abort(403, "You cannot edit your profile") form = UserChangePasswordForm() if form.validate_on_submit(): if not current_user.check_password(form.old_password.data): @@ -292,6 +294,9 @@ def password_reset(): # pylint: disable=W0613,W0612 if not user.active: # user should confirm email first return jsonify({"email": ["Account is not active"]}), 400 + if not user.can_edit_profile: + # using SSO + abort(403, "You cannot edit your profile") send_confirmation_email( current_app, @@ -344,6 +349,8 @@ def confirm_email(token): # pylint: disable=W0613,W0612 @auth_required def update_user_profile(): # pylint: disable=W0613,W0612 + if not current_user.can_edit_profile: + abort(403, "You cannot edit your profile") form = UserProfileDataForm.from_json(request.json) email_changed = current_user.email != form.email.data.strip() if not form.validate_on_submit(): @@ -427,6 +434,8 @@ def update_user(username): # pylint: disable=W0613,W0612 abort(400, "Unable to assign super admin role") user = User.query.filter_by(username=username).first_or_404("User not found") + if not user.can_edit_profile: + abort(403, "You cannot edit profile of this user") form.update_obj(user) # remove inactive since flag for ban or re-activation diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 8577070e..b7b5582b 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -239,6 +239,7 @@ def create( @property def can_edit_profile(self) -> bool: """Flag if we allow user to edit their email and name""" + # False when user is created by SSO login return self.passwd is not None From 2b30b49351ce697b40c57f2bb25b7030b785c6c9 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 1 Apr 2025 15:08:49 +0200 Subject: [PATCH 3/9] Allow creating mock of SSO user --- server/mergin/auth/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index b7b5582b..529b6816 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -41,8 +41,8 @@ class User(db.Model): db.Index("ix_user_email", func.lower(email), unique=True), ) - def __init__(self, username, email, passwd, is_admin=False): - self.username = username + def __init__(self, username, email, passwd=None, is_admin=False): + self.username = username or self.generate_username(email) self.email = email self.assign_password(passwd) self.is_admin = is_admin @@ -58,7 +58,7 @@ def check_password(self, password): def assign_password(self, password): if isinstance(password, str): password = password.encode("utf-8") - self.passwd = bcrypt.hashpw(password, bcrypt.gensalt()).decode("utf-8") + self.passwd = bcrypt.hashpw(password, bcrypt.gensalt()).decode("utf-8") if password else None @property def is_authenticated(self): From b66a3b652d5b10d20ba45da4804eba2c7da08e76 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 1 Apr 2025 15:09:02 +0200 Subject: [PATCH 4/9] Add test for admin --- server/mergin/tests/test_auth.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index a5217f6a..db5b9a85 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -453,6 +453,17 @@ def test_update_user(client): ) assert resp.status_code == 403 + # do not allow to update sso user + sso_user = User("", email="user@sso.com") + db.session.add(sso_user) + db.session.commit() + resp = client.patch( + url_for("/.mergin_auth_controller_update_user", username=sso_user.username), + data=json.dumps(data), + headers=json_headers, + ) + assert resp.status_code == 403 + def test_update_user_profile(client): login_as_admin(client) From a0849f8748d0384fd9759e2d90b81d3186d07093 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 1 Apr 2025 15:11:02 +0200 Subject: [PATCH 5/9] black . --- server/mergin/auth/models.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 529b6816..0eabe677 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -58,7 +58,11 @@ def check_password(self, password): def assign_password(self, password): if isinstance(password, str): password = password.encode("utf-8") - self.passwd = bcrypt.hashpw(password, bcrypt.gensalt()).decode("utf-8") if password else None + self.passwd = ( + bcrypt.hashpw(password, bcrypt.gensalt()).decode("utf-8") + if password + else None + ) @property def is_authenticated(self): From 781b0765bd33e8da92a08123a1f7cea2f5ebc943 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 1 Apr 2025 17:22:29 +0200 Subject: [PATCH 6/9] Add checks for others methods, address reviews --- server/mergin/auth/api.yaml | 2 ++ server/mergin/auth/controller.py | 15 ++++++++++----- server/mergin/auth/models.py | 4 ++-- server/mergin/tests/test_auth.py | 26 +++++++++++++++----------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index b42b2628..e0771689 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -466,6 +466,8 @@ paths: description: OK "400": $ref: "#/components/responses/BadStatusResp" + "403": + $ref: "#/components/responses/Forbidden" "404": $ref: "#/components/responses/NotFoundResp" /app/auth/confirm-email/{token}: diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 69ba2009..cdcbf735 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -40,6 +40,7 @@ EMAIL_CONFIRMATION_EXPIRATION = 12 * 3600 +CANNOT_EDIT_PROFILE_MSG = "You cannot edit profile of this user" # public endpoints @@ -256,7 +257,7 @@ def logout(): # pylint: disable=W0613,W0612 @auth_required def change_password(): # pylint: disable=W0613,W0612 if not current_user.can_edit_profile: - abort(403, "You cannot edit your profile") + abort(403, CANNOT_EDIT_PROFILE_MSG) form = UserChangePasswordForm() if form.validate_on_submit(): if not current_user.check_password(form.old_password.data): @@ -271,6 +272,8 @@ def change_password(): # pylint: disable=W0613,W0612 @auth_required def resend_confirm_email(): # pylint: disable=W0613,W0612 + if not current_user.can_edit_profile: + abort(403, CANNOT_EDIT_PROFILE_MSG) send_confirmation_email( current_app, current_user, @@ -296,7 +299,7 @@ def password_reset(): # pylint: disable=W0613,W0612 return jsonify({"email": ["Account is not active"]}), 400 if not user.can_edit_profile: # using SSO - abort(403, "You cannot edit your profile") + abort(403, CANNOT_EDIT_PROFILE_MSG) send_confirmation_email( current_app, @@ -316,6 +319,8 @@ def confirm_new_password(token): # pylint: disable=W0613,W0612 user = User.query.filter_by(email=email).first_or_404() if not user.active: abort(400, "Account is not active") + if not user.can_edit_profile: + abort(403, CANNOT_EDIT_PROFILE_MSG) form = UserPasswordForm.from_json(request.json) if form.validate(): @@ -336,6 +341,8 @@ def confirm_email(token): # pylint: disable=W0613,W0612 abort(400, "Invalid token") user = User.query.filter_by(email=email).first_or_404() + if not user.can_edit_profile: + abort(403, CANNOT_EDIT_PROFILE_MSG) if user.verified_email: return "", 200 @@ -350,7 +357,7 @@ def confirm_email(token): # pylint: disable=W0613,W0612 @auth_required def update_user_profile(): # pylint: disable=W0613,W0612 if not current_user.can_edit_profile: - abort(403, "You cannot edit your profile") + abort(403, CANNOT_EDIT_PROFILE_MSG) form = UserProfileDataForm.from_json(request.json) email_changed = current_user.email != form.email.data.strip() if not form.validate_on_submit(): @@ -434,8 +441,6 @@ def update_user(username): # pylint: disable=W0613,W0612 abort(400, "Unable to assign super admin role") user = User.query.filter_by(username=username).first_or_404("User not found") - if not user.can_edit_profile: - abort(403, "You cannot edit profile of this user") form.update_obj(user) # remove inactive since flag for ban or re-activation diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 0eabe677..7587a622 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -42,7 +42,7 @@ class User(db.Model): ) def __init__(self, username, email, passwd=None, is_admin=False): - self.username = username or self.generate_username(email) + self.username = username self.email = email self.assign_password(passwd) self.is_admin = is_admin @@ -244,7 +244,7 @@ def create( def can_edit_profile(self) -> bool: """Flag if we allow user to edit their email and name""" # False when user is created by SSO login - return self.passwd is not None + return self.passwd is not None and self.active class UserProfile(db.Model): diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index db5b9a85..ee866e94 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -453,17 +453,6 @@ def test_update_user(client): ) assert resp.status_code == 403 - # do not allow to update sso user - sso_user = User("", email="user@sso.com") - db.session.add(sso_user) - db.session.commit() - resp = client.patch( - url_for("/.mergin_auth_controller_update_user", username=sso_user.username), - data=json.dumps(data), - headers=json_headers, - ) - assert resp.status_code == 403 - def test_update_user_profile(client): login_as_admin(client) @@ -513,6 +502,21 @@ def test_update_user_profile(client): assert not user.verified_email assert user.email == "changed_email@mergin.co.uk" + # do not allow to update sso user + sso_user = add_user("sso_user", "sso") + login(client, sso_user.username, "sso") + sso_user.passwd = None + db.session.add(sso_user) + db.session.commit() + resp = client.post( + url_for("/.mergin_auth_controller_update_user_profile"), + data=json.dumps( + {"email": "changed_email@sso.co.uk"} + ), + headers=json_headers, + ) + assert resp.status_code == 403 + def test_search_user(client): user = User.query.filter_by(username="mergin").first() From a1093e4854352c0400d3e409fefdcb8524fcd583 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 2 Apr 2025 09:24:26 +0200 Subject: [PATCH 7/9] decorate can_edit_profile --- server/mergin/auth/app.py | 13 ++++++++++++- server/mergin/auth/controller.py | 4 ++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 7d3d8e29..309e73d7 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -11,7 +11,7 @@ from .commands import add_commands from .config import Configuration -from .models import User, UserProfile +from .models import User # signal for other versions to listen to user_account_closed = signal("user_account_closed") @@ -70,6 +70,17 @@ def wrapped_func(*args, **kwargs): return wrapped_func +def edit_profile_enabled(f): + from .controller import CANNOT_EDIT_PROFILE_MSG + + @functools.wraps(f) + def wrapped_func(*args, **kwargs): + if not current_user.can_edit_profile: + return CANNOT_EDIT_PROFILE_MSG, 403 + return f(*args, **kwargs) + return wrapped_func + + def authenticate(login, password): if "@" in login: query = func.lower(User.email) == func.lower(login) diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index cdcbf735..3845ca7d 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -20,6 +20,7 @@ generate_confirmation_token, user_created, user_account_closed, + edit_profile_enabled, ) from .bearer import encode_token from .models import User, LoginHistory, UserProfile @@ -255,6 +256,7 @@ def logout(): # pylint: disable=W0613,W0612 @auth_required +@edit_profile_enabled def change_password(): # pylint: disable=W0613,W0612 if not current_user.can_edit_profile: abort(403, CANNOT_EDIT_PROFILE_MSG) @@ -271,6 +273,7 @@ def change_password(): # pylint: disable=W0613,W0612 @auth_required +@edit_profile_enabled def resend_confirm_email(): # pylint: disable=W0613,W0612 if not current_user.can_edit_profile: abort(403, CANNOT_EDIT_PROFILE_MSG) @@ -355,6 +358,7 @@ def confirm_email(token): # pylint: disable=W0613,W0612 @auth_required +@edit_profile_enabled def update_user_profile(): # pylint: disable=W0613,W0612 if not current_user.can_edit_profile: abort(403, CANNOT_EDIT_PROFILE_MSG) From 341342b520969692c2d225c31c503ed15c5bccd4 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 2 Apr 2025 09:25:13 +0200 Subject: [PATCH 8/9] black . --- server/mergin/auth/app.py | 1 + server/mergin/tests/test_auth.py | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 309e73d7..029b4173 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -78,6 +78,7 @@ def wrapped_func(*args, **kwargs): if not current_user.can_edit_profile: return CANNOT_EDIT_PROFILE_MSG, 403 return f(*args, **kwargs) + return wrapped_func diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index ee866e94..cb4de697 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -510,9 +510,7 @@ def test_update_user_profile(client): db.session.commit() resp = client.post( url_for("/.mergin_auth_controller_update_user_profile"), - data=json.dumps( - {"email": "changed_email@sso.co.uk"} - ), + data=json.dumps({"email": "changed_email@sso.co.uk"}), headers=json_headers, ) assert resp.status_code == 403 From 2b09dd8e51ee207d1eddd8054ab2604a941b58b6 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 2 Apr 2025 10:12:56 +0200 Subject: [PATCH 9/9] Do not duplicate can_edit_profile check --- server/mergin/auth/app.py | 6 +++++- server/mergin/auth/controller.py | 8 +------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 029b4173..acfccf43 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -17,6 +17,8 @@ user_account_closed = signal("user_account_closed") user_created = signal("user_created") +CANNOT_EDIT_PROFILE_MSG = "You cannot edit profile of this user" + def register(app): """Register mergin auth module in Flask app @@ -71,10 +73,12 @@ def wrapped_func(*args, **kwargs): def edit_profile_enabled(f): - from .controller import CANNOT_EDIT_PROFILE_MSG + """Decorator to check if user can edit their profile (it is not allowed for SSO users)""" @functools.wraps(f) def wrapped_func(*args, **kwargs): + if not current_user or not current_user.is_authenticated: + return "Authentication information is missing or invalid.", 401 if not current_user.can_edit_profile: return CANNOT_EDIT_PROFILE_MSG, 403 return f(*args, **kwargs) diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 3845ca7d..3e00ce16 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -21,6 +21,7 @@ user_created, user_account_closed, edit_profile_enabled, + CANNOT_EDIT_PROFILE_MSG, ) from .bearer import encode_token from .models import User, LoginHistory, UserProfile @@ -41,7 +42,6 @@ EMAIL_CONFIRMATION_EXPIRATION = 12 * 3600 -CANNOT_EDIT_PROFILE_MSG = "You cannot edit profile of this user" # public endpoints @@ -258,8 +258,6 @@ def logout(): # pylint: disable=W0613,W0612 @auth_required @edit_profile_enabled def change_password(): # pylint: disable=W0613,W0612 - if not current_user.can_edit_profile: - abort(403, CANNOT_EDIT_PROFILE_MSG) form = UserChangePasswordForm() if form.validate_on_submit(): if not current_user.check_password(form.old_password.data): @@ -275,8 +273,6 @@ def change_password(): # pylint: disable=W0613,W0612 @auth_required @edit_profile_enabled def resend_confirm_email(): # pylint: disable=W0613,W0612 - if not current_user.can_edit_profile: - abort(403, CANNOT_EDIT_PROFILE_MSG) send_confirmation_email( current_app, current_user, @@ -360,8 +356,6 @@ def confirm_email(token): # pylint: disable=W0613,W0612 @auth_required @edit_profile_enabled def update_user_profile(): # pylint: disable=W0613,W0612 - if not current_user.can_edit_profile: - abort(403, CANNOT_EDIT_PROFILE_MSG) form = UserProfileDataForm.from_json(request.json) email_changed = current_user.email != form.email.data.strip() if not form.validate_on_submit():