From 4d34f5d5f98a7d0e3ba08a09aa1e6f3ebd3fe48d Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Fri, 29 Nov 2024 08:32:54 +0100 Subject: [PATCH 1/6] Roles refactor: New public API with reworked project roles --- server/mergin/app.py | 1 - server/mergin/auth/api.yaml | 53 ++++++ server/mergin/auth/app.py | 15 +- server/mergin/auth/controller.py | 49 +++++- server/mergin/auth/models.py | 47 ++++- server/mergin/auth/tasks.py | 2 +- server/mergin/sync/interfaces.py | 29 ++++ server/mergin/sync/models.py | 22 +++ server/mergin/sync/public_api_controller.py | 4 +- server/mergin/sync/public_api_v2.yaml | 160 ++++++++++++++++++ .../mergin/sync/public_api_v2_controller.py | 108 +++++++++--- server/mergin/sync/schemas.py | 12 +- server/mergin/sync/workspace.py | 37 ++-- server/mergin/tests/test_auth.py | 56 ++++++ server/mergin/tests/test_public_api_v2.py | 63 ++++++- server/mergin/tests/test_workspace.py | 3 +- 16 files changed, 601 insertions(+), 60 deletions(-) diff --git a/server/mergin/app.py b/server/mergin/app.py index 751b6561..1c767137 100644 --- a/server/mergin/app.py +++ b/server/mergin/app.py @@ -177,7 +177,6 @@ def create_app(public_keys: List[str] = None) -> Flask: arguments={"title": "Mergin"}, options={"swagger_ui": Configuration.SWAGGER_UI}, validate_responses=True, - pythonic_params=True, ) app.add_api( "sync/private_api.yaml", diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index f89e110d..90891a16 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -10,6 +10,8 @@ tags: description: Mergin user - name: admin description: For mergin admin + - name: public + description: Public API paths: /app/auth/user/search: get: @@ -620,6 +622,57 @@ paths: $ref: "#/components/responses/UnauthorizedError" "403": $ref: "#/components/responses/Forbidden" + /v2/users: + post: + tags: + - user + - public + summary: Create user + operationId: create_user + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - email + - password + - workspace_id + - role + properties: + email: + type: string + format: email + example: john.doe@example.com + username: + type: string + example: john.doe + password: + type: string + format: password + example: topsecret + workspace_id: + type: integer + example: 1 + role: + $ref: "#/components/schemas/WorkspaceRole" + notify_user: + type: boolean + responses: + "201": + description: User info + content: + application/json: + schema: + $ref: "#/components/schemas/UserInfo" + "401": + $ref: "#/components/responses/UnauthorizedError" + "404": + $ref: "#/components/responses/NotFoundResp" + "422": + $ref: "#/components/responses/UnprocessableEntity" + x-openapi-router-controller: mergin.auth.controller components: responses: UnauthorizedError: diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 1a3caba4..10f9270c 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -12,7 +12,6 @@ from .commands import add_commands from .config import Configuration from .models import User, UserProfile -from ..app import db # signal for other versions to listen to user_account_closed = signal("user_account_closed") @@ -97,12 +96,14 @@ def confirm_token(token, expiration=3600 * 24 * 3): return email -def send_confirmation_email(app, user, url, template, header): +def send_confirmation_email(app, user, url, template, header, **kwargs): from ..celery import send_email_async token = generate_confirmation_token(app, user.email) confirm_url = f"{url}/{token}" - html = render_template(template, subject=header, confirm_url=confirm_url, user=user) + html = render_template( + template, subject=header, confirm_url=confirm_url, user=user, **kwargs + ) email_data = { "subject": header, "html": html, @@ -110,11 +111,3 @@ def send_confirmation_email(app, user, url, template, header): "sender": app.config["MAIL_DEFAULT_SENDER"], } send_email_async.delay(**email_data) - - -def do_register_user(username, email, password): - user = User(username.strip(), email.strip(), password, False) - user.profile = UserProfile() - db.session.add(user) - db.session.commit() - return user diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index cdea3181..a39d0f26 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -15,7 +15,6 @@ from .app import ( auth_required, authenticate, - do_register_user, send_confirmation_email, confirm_token, generate_confirmation_token, @@ -91,7 +90,7 @@ def get_user_public(username=None): # noqa: E501 "storage": user_workspace.storage if user_workspace else 104857600, "storage_limit": user_workspace.storage if user_workspace else 104857600, "organisations": { - ws.name: ws.get_user_role(current_user) for ws in all_workspaces + ws.name: ws.get_user_role(current_user).value for ws in all_workspaces }, } return user_info, 200 @@ -373,7 +372,7 @@ def register_user(): # pylint: disable=W0613,W0612 form = UserRegistrationForm() if form.validate(): - user = do_register_user(form.username.data, form.email.data, form.password.data) + user = User.create(form.username.data, form.email.data, form.password.data) user_created.send(user, source="admin") token = generate_confirmation_token(current_app, user.email) confirm_url = f"confirm-email/{token}" @@ -485,7 +484,7 @@ def get_user_info(): user_info = UserInfoSchema().dump(current_user) workspaces = current_app.ws_handler.list_user_workspaces(current_user.username) user_info["workspaces"] = [ - {"id": ws.id, "name": ws.name, "role": ws.get_user_role(current_user)} + {"id": ws.id, "name": ws.name, "role": ws.get_user_role(current_user).value} for ws in workspaces ] preferred_workspace = current_app.ws_handler.get_preferred(current_user) @@ -498,3 +497,45 @@ def get_user_info(): for inv in invitations ] return user_info, 200 + + +@auth_required +def create_user(): + """Create new user""" + workspace = current_app.ws_handler.get(request.json.get("workspace_id")) + if not (workspace and workspace.can_add_users(current_user)): + abort(403) + + username = request.json.get( + "username", User.generate_username(request.json["email"]) + ) + form = UserRegistrationForm() + form.confirm.data = form.password.data + form.username.data = username + if not form.validate(): + return jsonify(form.errors), 400 + + user = User.create( + form.username.data, + form.email.data, + form.password.data, + request.json.get("notify_user", False), + ) + user_created.send( + user, + source="api", + workspace_id=request.json["workspace_id"], + workspace_role=request.json["role"], + ) + + if user.profile.receive_notifications: + send_confirmation_email( + current_app, + user, + "confirm-email", + "email/user_created.html", + "Invitation to Mergin Maps", + password=form.password.data, + ) + + return jsonify(UserInfoSchema().dump(user)), 201 diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 622c1c2d..991698c0 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -7,7 +7,7 @@ from typing import List, Optional import bcrypt from flask import current_app, request -from sqlalchemy import or_, func +from sqlalchemy import or_, func, text from ..app import db from ..sync.models import ProjectUser @@ -179,6 +179,51 @@ def anonymize(self): self.profile.last_name = None db.session.commit() + @classmethod + def get_by_login(cls, username: str) -> Optional[User]: + """Find user by its username or email""" + username = username.strip().lower() + return cls.query.filter( + or_( + func.lower(User.email) == username, + func.lower(User.username) == username, + ) + ).first() + + @classmethod + def generate_username(cls, email: str) -> Optional[str]: + """Autogenerate username from email""" + if not "@" in email: + return + username = email.split("@")[0].strip().lower() + # check if we already do not have existing usernames + suffix = db.session.execute( + text( + """ + SELECT + replace(username, :username, '0')::int AS suffix + FROM "user" + WHERE + username = :username OR + username SIMILAR TO :username'\d+' + ORDER BY replace(username, :username, '0')::int DESC + LIMIT 1; + """ + ), + {"username": username}, + ).scalar() + return username if suffix is None else username + str(int(suffix) + 1) + + @classmethod + def create( + cls, username: str, email: str, password: str, notifications: bool = True + ) -> User: + user = cls(username.strip(), email.strip(), password, False) + user.profile = UserProfile(receive_notifications=notifications) + db.session.add(user) + db.session.commit() + return user + class UserProfile(db.Model): user_id = db.Column( diff --git a/server/mergin/auth/tasks.py b/server/mergin/auth/tasks.py index a09848a8..e18cc5d4 100644 --- a/server/mergin/auth/tasks.py +++ b/server/mergin/auth/tasks.py @@ -6,7 +6,7 @@ from sqlalchemy.sql.operators import isnot from ..celery import celery -from .app import db +from ..app import db from .models import User from .config import Configuration diff --git a/server/mergin/sync/interfaces.py b/server/mergin/sync/interfaces.py index 4cf12650..3961ee37 100644 --- a/server/mergin/sync/interfaces.py +++ b/server/mergin/sync/interfaces.py @@ -3,6 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial from abc import ABC, abstractmethod +from enum import Enum class AbstractWorkspace: @@ -74,6 +75,16 @@ def project_count(self): """Return number of workspace projects""" pass + @abstractmethod + def members(self): + """Return workspace members""" + pass + + @abstractmethod + def can_add_users(self, user): + """Check if user can add another user to workspace""" + pass + class WorkspaceHandler(ABC): """ @@ -169,3 +180,21 @@ def get_push_permission(self, changes: dict): Return project permission for user to push data to project """ pass + + +class WorkspaceRole(Enum): + GUEST = "guest" + READER = "reader" + EDITOR = "editor" + WRITER = "writer" + ADMIN = "admin" + OWNER = "owner" + + @classmethod + def values(cls): + return [member.value for member in cls.__members__.values()] + + def __ge__(self, other): + """Compare roles""" + members = list(WorkspaceRole.__members__) + return members.index(self.name) >= members.index(other.name) diff --git a/server/mergin/sync/models.py b/server/mergin/sync/models.py index e7c69159..de915706 100644 --- a/server/mergin/sync/models.py +++ b/server/mergin/sync/models.py @@ -28,6 +28,7 @@ ChangesSchema, ProjectFile, ) +from .interfaces import WorkspaceRole from .storages.disk import move_to_tmp from ..app import db from .storages import DiskStorage @@ -281,6 +282,18 @@ def unset_role(self, user_id: int) -> None: if member: self.project_users.remove(member) + def get_member(self, user_id: int) -> Optional[ProjectMember]: + """Get project member""" + member = self._member(user_id) + if member: + return ProjectMember( + id=user_id, + username=member.user.username, + email=member.user.email, + project_role=ProjectRole(member.role), + workspace_role=self.workspace.get_user_role(member.user), + ) + def members_by_role(self, role: ProjectRole) -> List[int]: """Project members' ids with at least required role (or higher)""" return [u.user_id for u in self.project_users if ProjectRole(u.role) >= role] @@ -331,6 +344,15 @@ def __lt__(self, other): return members.index(self.name) < members.index(other.name) +@dataclass +class ProjectMember: + id: int + email: str + username: str + workspace_role: WorkspaceRole + project_role: Optional[ProjectRole] + + @dataclass class ProjectAccessDetail: id: int or str diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index a325ef67..a0eff09e 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -34,6 +34,8 @@ from sqlalchemy.orm import load_only from werkzeug.exceptions import HTTPException + +from .interfaces import WorkspaceRole from ..app import db from ..auth import auth_required from ..auth.models import User @@ -1427,7 +1429,7 @@ def get_workspace_by_id(id): if not ( ws.user_has_permissions(current_user, "read") - or ws.get_user_role(current_user) == "guest" + or ws.get_user_role(current_user) == WorkspaceRole.GUEST ): abort(403, f"You do not have permissions to workspace") diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml index 4973696a..dcb9a3ee 100644 --- a/server/mergin/sync/public_api_v2.yaml +++ b/server/mergin/sync/public_api_v2.yaml @@ -98,6 +98,129 @@ paths: "404": $ref: "#/components/responses/NotFound" x-openapi-router-controller: mergin.sync.public_api_v2_controller + /projects/{id}/collaborators: + parameters: + - $ref: "#/components/parameters/ProjectId" + get: + tags: + - project + summary: Get project collaborators + operationId: get_project_members + responses: + "200": + description: Project members + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/ProjectMember" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFound" + x-openapi-router-controller: mergin.sync.public_api_v2_controller + post: + tags: + - project + summary: Add project collaborator + operationId: add_project_member + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - username + - role + properties: + username: + type: string + example: john.doe + description: username or email + role: + $ref: "#/components/schemas/ProjectRole" + responses: + "201": + description: New project member + content: + application/json: + schema: + $ref: "#/components/schemas/ProjectMember" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFound" + x-openapi-router-controller: mergin.sync.public_api_v2_controller + /projects/{id}/collaborators/{user_id}: + parameters: + - $ref: "#/components/parameters/ProjectId" + - name: user_id + in: path + description: user id + required: true + schema: + type: integer + patch: + tags: + - project + summary: Update project collaborator + operationId: update_project_member + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - role + properties: + role: + $ref: "#/components/schemas/ProjectRole" + responses: + "200": + description: Updated project member + content: + application/json: + schema: + $ref: "#/components/schemas/ProjectMember" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFound" + x-openapi-router-controller: mergin.sync.public_api_v2_controller + delete: + tags: + - project + summary: Remove project collaborator + operationId: remove_project_member + parameters: + - $ref: "#/components/parameters/ProjectId" + responses: + "204": + $ref: "#/components/responses/NoContent" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFound" + x-openapi-router-controller: mergin.sync.public_api_v2_controller components: responses: NoContent: @@ -122,3 +245,40 @@ components: type: string format: uuid pattern: \b[0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f]{12}\b + schemas: + ProjectRole: + type: string + nullable: true + enum: + - owner + - writer + - editor + - reader + example: reader + WorkspaceRole: + type: string + enum: + - owner + - admin + - writer + - editor + - reader + - guest + example: writer + ProjectMember: + type: object + properties: + id: + type: integer + example: 1 + username: + type: string + example: john.doe + email: + type: string + format: email + example: john.doe@example.com + workspace_role: + $ref: '#/components/schemas/WorkspaceRole' + project_role: + $ref: '#/components/schemas/ProjectRole' diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py index 59cdbb2d..be1eb8ee 100644 --- a/server/mergin/sync/public_api_v2_controller.py +++ b/server/mergin/sync/public_api_v2_controller.py @@ -3,28 +3,28 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial from datetime import datetime - from connexion import NoContent, request from flask import abort, jsonify from flask_login import current_user -from mergin.app import db -from mergin.auth import auth_required -from mergin.sync.models import Project -from mergin.sync.permissions import ProjectPermissions, require_project_by_uuid -from mergin.sync.utils import is_name_allowed +from .schemas import ProjectMemberSchema +from .workspace import WorkspaceRole +from ..app import db +from ..auth import auth_required +from ..auth.models import User +from .models import Project, ProjectRole, ProjectMember +from .permissions import ProjectPermissions, require_project_by_uuid +from .private_api_controller import project_access_granted +from .utils import is_name_allowed @auth_required -def schedule_delete_project(id_): +def schedule_delete_project(id): """Schedule deletion of the project. Celery job `mergin.sync.tasks.remove_projects_backups` does the rest. - - :param id_: Project id - :type id_: str """ - project = require_project_by_uuid(id_, ProjectPermissions.Delete) + project = require_project_by_uuid(id, ProjectPermissions.Delete) project.removed_at = datetime.utcnow() project.removed_by = current_user.id db.session.commit() @@ -33,26 +33,18 @@ def schedule_delete_project(id_): @auth_required -def delete_project_now(id_): - """Delete the project immediately. - - :param id_: Project id - :type id_: str - """ - project = require_project_by_uuid(id_, ProjectPermissions.Delete, scheduled=True) +def delete_project_now(id): + """Delete the project immediately""" + project = require_project_by_uuid(id, ProjectPermissions.Delete, scheduled=True) project.delete() return NoContent, 204 @auth_required -def update_project(id_): - """Rename project - - :param id_: Project_id - :type id_: str - """ - project = require_project_by_uuid(id_, ProjectPermissions.Update) +def update_project(id): + """Rename project""" + project = require_project_by_uuid(id, ProjectPermissions.Update) new_name = request.json["name"] if not is_name_allowed(new_name): @@ -72,3 +64,69 @@ def update_project(id_): db.session.commit() return NoContent, 204 + + +@auth_required +def get_project_members(id): + """Get project collaborators, with both direct role and inherited role from workspace""" + project = require_project_by_uuid(id, ProjectPermissions.Update) + project_members = [] + for user, workspace_role in project.workspace.members(): + project_role = project.get_role(user.id) + if workspace_role != WorkspaceRole.GUEST or project_role is not None: + project_members.append( + ProjectMember( + id=user.id, + username=user.username, + email=user.email, + project_role=project_role, + workspace_role=workspace_role, + ) + ) + + data = ProjectMemberSchema(many=True).dump(project_members) + return data, 200 + + +@auth_required +def add_project_member(id): + """Add project collaborator""" + project = require_project_by_uuid(id, ProjectPermissions.Update) + user = User.get_by_login(request.json["username"]) + if not user: + abort(404) + + if project.get_role(user.id): + abort(409, "User is already a project member") + + project.set_role(user.id, ProjectRole(request.json["role"])) + project_access_granted.send(project, user_id=user.id) + db.session.commit() + data = ProjectMemberSchema().dump(project.get_member(user.id)) + return data, 201 + + +@auth_required +def update_project_member(id, user_id): + """Update project collaborator""" + project = require_project_by_uuid(id, ProjectPermissions.Update) + user = User.query.filter_by(id=user_id, active=True).first_or_404() + if not project.get_role(user_id): + abort(404, "User is not a project member") + + project.set_role(user.id, ProjectRole(request.json["role"])) + db.session.commit() + data = ProjectMemberSchema().dump(project.get_member(user.id)) + return data, 200 + + +@auth_required +def remove_project_member(id, user_id): + """Remove project collaborator""" + project = require_project_by_uuid(id, ProjectPermissions.Update) + if not project.get_role(user_id): + abort(404, "User is not a project member") + + project.unset_role(user_id) + db.session.commit() + return NoContent, 204 diff --git a/server/mergin/sync/schemas.py b/server/mergin/sync/schemas.py index 5ad0629e..1225c4d5 100644 --- a/server/mergin/sync/schemas.py +++ b/server/mergin/sync/schemas.py @@ -16,7 +16,9 @@ FileHistory, PushChangeType, ProjectRole, + ProjectUser, ) +from .workspace import WorkspaceRole from ..app import DateTimeWithZ, ma from ..auth.models import User @@ -335,7 +337,7 @@ class UserWorkspaceSchema(ma.SQLAlchemyAutoSchema): def _user_role(self, obj): if not self.context.get("user"): return - return obj.get_user_role(self.context.get("user")) + return obj.get_user_role(self.context.get("user")).value class ProjectInvitationAccessSchema(Schema): @@ -393,3 +395,11 @@ class Meta: "user_agent", ] load_instance = True + + +class ProjectMemberSchema(Schema): + id = fields.Integer() + username = fields.String() + email = fields.Email() + project_role = fields.Enum(enum=ProjectRole, by_value=True) + workspace_role = fields.Enum(enum=WorkspaceRole, by_value=True) diff --git a/server/mergin/sync/workspace.py b/server/mergin/sync/workspace.py index be0d6ac5..664aecf7 100644 --- a/server/mergin/sync/workspace.py +++ b/server/mergin/sync/workspace.py @@ -19,7 +19,7 @@ from ..app import db from ..auth.models import User from ..config import Configuration -from .interfaces import AbstractWorkspace, WorkspaceHandler +from .interfaces import AbstractWorkspace, WorkspaceHandler, WorkspaceRole class GlobalWorkspace(AbstractWorkspace): @@ -72,33 +72,33 @@ def disk_usage(self): def user_has_permissions(self, user, permissions): role = self.get_user_role(user) # mergin super-user has all permissions - if role == "owner": + if role is WorkspaceRole.OWNER: return True if permissions == "read": - return role in ["admin", "writer", "editor", "reader"] + return role >= WorkspaceRole.READER elif permissions == "edit": - return role in ["admin", "writer", "editor"] + return role >= WorkspaceRole.EDITOR elif permissions == "write": - return role in ["admin", "writer"] + return role >= WorkspaceRole.WRITER elif permissions == "admin": - return role == "admin" + return role >= WorkspaceRole.ADMIN else: return False def user_is_member(self, user): return True - def get_user_role(self, user): + def get_user_role(self, user) -> WorkspaceRole: if user.is_admin: - return "owner" + return WorkspaceRole.OWNER if Configuration.GLOBAL_ADMIN: - return "admin" + return WorkspaceRole.ADMIN if Configuration.GLOBAL_WRITE: - return "writer" + return WorkspaceRole.WRITER if Configuration.GLOBAL_READ: - return "reader" - return "guest" + return WorkspaceRole.READER + return WorkspaceRole.GUEST def project_count(self): from .models import Project @@ -110,6 +110,17 @@ def project_count(self): .count() ) + def members(self): + return [ + (user, self.get_user_role(user)) + for user in User.query.filter(User.active.is_(True)) + .order_by(User.email) + .all() + ] + + def can_add_users(self, user: User) -> bool: + return user.is_admin + class GlobalWorkspaceHandler(WorkspaceHandler): """Implements handler for GlobalWorkspace objects""" @@ -317,7 +328,7 @@ def project_access(self, project: Project) -> List[ProjectAccessDetail]: member = ProjectAccessDetail( id=dm.id, username=dm.username, - role=ws.get_user_role(dm), + role=ws.get_user_role(dm).value, name=dm.profile.name(), email=dm.email, project_permission=project_role and project_role.value, diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index 90ff0ce7..2613c68f 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -782,3 +782,59 @@ def test_update_project_v2(client): login_as_admin(client) resp = client.patch(f"v2/projects/{project.id}", json=data) assert resp.status_code == 204 + + +@patch("mergin.celery.send_email_async.apply_async") +def test_add_user(send_email_mock, client): + """Test adding new user via public endpoint""" + login_as_admin(client) + data = { + "username": "test_user", + "email": "test@test.com", + "password": "#pwd1234", + "role": "reader", + "workspace_id": test_workspace_id, + } + response = client.post("/v2/users", json=data) + assert response.status_code == 201 + assert response.json["username"] == "test_user" + assert response.json["email"] == "test@test.com" + assert response.json["receive_notifications"] is False + assert not send_email_mock.called + + # drop required attribute + data.pop("email") + assert client.post("/v2/users", json=data).status_code == 400 + + # try register with the same account again + data["email"] = "test@test.com" + assert client.post("/v2/users", json=data).status_code == 400 + + # register with optional flag sent but username missing + data["email"] = "test2@test.com" + data.pop("username") + data["notify_user"] = True + response = client.post("/v2/users", json=data) + assert response.status_code == 201 + assert response.json["receive_notifications"] is True + assert response.json["username"] == "test2" # autogenerated from email + assert send_email_mock.called + assert send_email_mock.call_args.args[1]["subject"] == "Invitation to Mergin Maps" + + # ordinary user cannot add others + login(client, "test2", "#pwd1234") + assert client.post("/v2/users", json=data).status_code == 403 + + +def test_username_generation(client): + """Test generation of username from email""" + assert User.generate_username("tralala@example.com") == "tralala" + assert User.generate_username("TraLala@example.com") == "tralala" + assert User.generate_username("tralala") is None + + # add user to test autoincrement + user = add_user("user", "user") + assert User.generate_username(user.email) == user.username + "1" + + user = add_user("user25", "user") + assert User.generate_username(user.email) == user.username + "1" diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py index 4bdb65a2..6dc96610 100644 --- a/server/mergin/tests/test_public_api_v2.py +++ b/server/mergin/tests/test_public_api_v2.py @@ -1,11 +1,14 @@ # Copyright (C) Lutra Consulting Limited # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial - +from .utils import add_user from ..app import db from mergin.sync.models import Project from tests import test_project, test_workspace_id +from ..config import Configuration +from ..sync.models import ProjectRole + def test_schedule_delete_project(client): project = Project.query.filter_by( @@ -65,3 +68,61 @@ def test_rename_project(client): ) assert response.status_code == 400 assert response.json["code"] == "InvalidProjectName" + + +def test_project_members(client): + """Test CRUD endpoints for direct project members""" + project = Project.query.filter_by( + workspace_id=test_workspace_id, name=test_project + ).first() + url = f"v2/projects/{project.id}/collaborators" + response = client.get(url) + assert response.status_code == 200 + assert len(response.json) == 1 + + user = add_user("user", "password") + # user not a member yet + response = client.delete(url + f"/{user.id}") + assert response.status_code == 404 + + role = ProjectRole.READER.value + response = client.patch(url + f"/{user.id}", json={"role": role}) + assert response.status_code == 404 + + # add direct access + response = client.post(url, json={"role": role, "username": user.email}) + assert response.status_code == 201 + assert response.json["id"] == user.id + assert response.json["project_role"] == role + + Configuration.GLOBAL_READ = 0 + Configuration.GLOBAL_WRITE = 0 + Configuration.GLOBAL_ADMIN = 0 + response = client.get(url) + assert len(response.json) == 2 + member = next(u for u in response.json if u["id"] == user.id) + assert member["project_role"] == role + assert member["workspace_role"] == "guest" + + response = client.patch( + url + f"/{user.id}", json={"role": ProjectRole.WRITER.value} + ) + assert response.status_code == 200 + assert response.json["project_role"] == ProjectRole.WRITER.value + + response = client.delete(url + f"/{user.id}") + assert response.status_code == 204 + + response = client.get(url) + assert len(response.json) == 1 + + # test access only by workspace role + Configuration.GLOBAL_READ = 1 + response = client.get(url) + member = next(u for u in response.json if u["id"] == user.id) + assert not member["project_role"] + assert member["workspace_role"] == "reader" + + # access provided by workspace role cannot be removed directly + response = client.delete(url + f"/{user.id}") + assert response.status_code == 404 diff --git a/server/mergin/tests/test_workspace.py b/server/mergin/tests/test_workspace.py index a10eadb9..29ed6c5f 100644 --- a/server/mergin/tests/test_workspace.py +++ b/server/mergin/tests/test_workspace.py @@ -8,6 +8,7 @@ from ..app import db from ..config import Configuration +from ..sync.interfaces import WorkspaceRole from ..sync.models import FileHistory, ProjectVersion, PushChangeType, ProjectFilePath from ..sync.workspace import GlobalWorkspaceHandler from .utils import add_user, login, create_project @@ -25,7 +26,7 @@ def test_workspace_implementation(client): Configuration.GLOBAL_READ = False Configuration.GLOBAL_WRITE = False Configuration.GLOBAL_ADMIN = False - assert ws.get_user_role(user) == "guest" + assert ws.get_user_role(user) == WorkspaceRole.GUEST assert not ws.user_has_permissions(user, "owner") assert not ws.user_has_permissions(user, "read") assert not ws.user_has_permissions(user, "write") From 1b66b148ec7f33cb3ec13448bebed9f90b2d0b1c Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 6 Dec 2024 09:41:13 +0100 Subject: [PATCH 2/6] Create project fix --- server/mergin/sync/public_api_controller.py | 1 - 1 file changed, 1 deletion(-) diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index a0eff09e..68563779 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -222,7 +222,6 @@ def add_project(namespace): # noqa: E501 **request.json, creator=current_user, workspace=workspace, - public=request.json.get("public", False), ) p.updated = datetime.utcnow() From 4578903d507ee843f1aab68b8c1893c638619097 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 6 Dec 2024 09:54:12 +0100 Subject: [PATCH 3/6] Be safe to have mandatory public attribute --- server/mergin/sync/public_api_controller.py | 1 + 1 file changed, 1 insertion(+) diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index 68563779..b460371e 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -216,6 +216,7 @@ def add_project(namespace): # noqa: E501 request.json["storage_params"] = { "type": "local", "location": generate_location(), + "public": request.json.get("public", False), } p = Project( From be604ea7a9dac4843c1f88c1e61dd035a099b5ec Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Mon, 9 Dec 2024 11:36:10 +0100 Subject: [PATCH 4/6] Add test for public argument --- server/mergin/sync/public_api_controller.py | 1 - .../mergin/tests/test_project_controller.py | 20 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index b460371e..68563779 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -216,7 +216,6 @@ def add_project(namespace): # noqa: E501 request.json["storage_params"] = { "type": "local", "location": generate_location(), - "public": request.json.get("public", False), } p = Project( diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py index 21387c19..680efe95 100644 --- a/server/mergin/tests/test_project_controller.py +++ b/server/mergin/tests/test_project_controller.py @@ -449,6 +449,26 @@ def test_add_project(client, app, data, expected): ) +project_data = [ + ( + {"name": "project1", "public": True}, + True, + ), # project accessibility follows the request argument + ({"name": "project1"}, False), # by default the project is private +] + + +@pytest.mark.parametrize("add_project_data,public", project_data) +def test_add_public_project(client, add_project_data, public): + resp = client.post( + "/v1/project/{}".format(test_workspace_name), + data=json.dumps(add_project_data), + headers=json_headers, + ) + p = Project.query.filter_by(name=add_project_data["name"]).first() + assert p.public == public + + def test_versioning(client): # tests if blank project has version set up to v0 resp = client.post( From 56cd00883904c52e2a0ba02664e6b76b7500320f Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Mon, 9 Dec 2024 14:14:44 +0100 Subject: [PATCH 5/6] Address review comments --- server/mergin/auth/app.py | 4 ++++ server/mergin/auth/models.py | 10 +++++----- server/mergin/sync/private_api_controller.py | 2 +- server/mergin/sync/public_api_v2.yaml | 2 +- server/mergin/sync/public_api_v2_controller.py | 4 ++-- server/mergin/sync/schemas.py | 3 ++- 6 files changed, 15 insertions(+), 10 deletions(-) diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 10f9270c..cfba8d98 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -97,6 +97,10 @@ def confirm_token(token, expiration=3600 * 24 * 3): def send_confirmation_email(app, user, url, template, header, **kwargs): + """ + Send confirmation email from selected template with customizable email subject and confirmation URL. + Optional kwargs are passed to render_template method if needed for particular template. + """ from ..celery import send_email_async token = generate_confirmation_token(app, user.email) diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 991698c0..c16c21f6 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -180,13 +180,13 @@ def anonymize(self): db.session.commit() @classmethod - def get_by_login(cls, username: str) -> Optional[User]: - """Find user by its username or email""" - username = username.strip().lower() + def get_by_login(cls, login: str) -> Optional[User]: + """Find user by its login which can be either username or email""" + login = login.strip().lower() return cls.query.filter( or_( - func.lower(User.email) == username, - func.lower(User.username) == username, + func.lower(User.email) == login, + func.lower(User.username) == login, ) ).first() diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index d4b66c0d..616ac69d 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -135,10 +135,10 @@ def accept_project_access_request(request_id): project = access_request.project project_role = ProjectPermissions.get_user_project_role(project, current_user) if project_role == ProjectRole.OWNER: + access_request.accept(permission) project_access_granted.send( access_request.project, user_id=access_request.requested_by ) - access_request.accept(permission) return "", 200 abort(403, "You don't have permissions to accept project access request") diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml index dcb9a3ee..16c71155 100644 --- a/server/mergin/sync/public_api_v2.yaml +++ b/server/mergin/sync/public_api_v2.yaml @@ -206,7 +206,7 @@ paths: tags: - project summary: Remove project collaborator - operationId: remove_project_member + operationId: remove_project_collaborator parameters: - $ref: "#/components/parameters/ProjectId" responses: diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py index be1eb8ee..c4e3cd77 100644 --- a/server/mergin/sync/public_api_v2_controller.py +++ b/server/mergin/sync/public_api_v2_controller.py @@ -100,8 +100,8 @@ def add_project_member(id): abort(409, "User is already a project member") project.set_role(user.id, ProjectRole(request.json["role"])) - project_access_granted.send(project, user_id=user.id) db.session.commit() + project_access_granted.send(project, user_id=user.id) data = ProjectMemberSchema().dump(project.get_member(user.id)) return data, 201 @@ -121,7 +121,7 @@ def update_project_member(id, user_id): @auth_required -def remove_project_member(id, user_id): +def remove_project_collaborator(id, user_id): """Remove project collaborator""" project = require_project_by_uuid(id, ProjectPermissions.Update) if not project.get_role(user_id): diff --git a/server/mergin/sync/schemas.py b/server/mergin/sync/schemas.py index 1225c4d5..c782c5ca 100644 --- a/server/mergin/sync/schemas.py +++ b/server/mergin/sync/schemas.py @@ -337,7 +337,8 @@ class UserWorkspaceSchema(ma.SQLAlchemyAutoSchema): def _user_role(self, obj): if not self.context.get("user"): return - return obj.get_user_role(self.context.get("user")).value + role = obj.get_user_role(self.context.get("user")) + return role and role.value class ProjectInvitationAccessSchema(Schema): From dfa0d463877718212562928c0ce4ddcc0b6d4a43 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Mon, 9 Dec 2024 15:50:17 +0100 Subject: [PATCH 6/6] Rename functions to match API naming --- server/mergin/sync/public_api_v2.yaml | 6 +++--- server/mergin/sync/public_api_v2_controller.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml index 16c71155..a167cb26 100644 --- a/server/mergin/sync/public_api_v2.yaml +++ b/server/mergin/sync/public_api_v2.yaml @@ -105,7 +105,7 @@ paths: tags: - project summary: Get project collaborators - operationId: get_project_members + operationId: get_project_collaborators responses: "200": description: Project members @@ -128,7 +128,7 @@ paths: tags: - project summary: Add project collaborator - operationId: add_project_member + operationId: add_project_collaborator requestBody: required: true content: @@ -174,7 +174,7 @@ paths: tags: - project summary: Update project collaborator - operationId: update_project_member + operationId: update_project_collaborator requestBody: required: true content: diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py index c4e3cd77..d2076910 100644 --- a/server/mergin/sync/public_api_v2_controller.py +++ b/server/mergin/sync/public_api_v2_controller.py @@ -67,7 +67,7 @@ def update_project(id): @auth_required -def get_project_members(id): +def get_project_collaborators(id): """Get project collaborators, with both direct role and inherited role from workspace""" project = require_project_by_uuid(id, ProjectPermissions.Update) project_members = [] @@ -89,7 +89,7 @@ def get_project_members(id): @auth_required -def add_project_member(id): +def add_project_collaborator(id): """Add project collaborator""" project = require_project_by_uuid(id, ProjectPermissions.Update) user = User.get_by_login(request.json["username"]) @@ -107,7 +107,7 @@ def add_project_member(id): @auth_required -def update_project_member(id, user_id): +def update_project_collaborator(id, user_id): """Update project collaborator""" project = require_project_by_uuid(id, ProjectPermissions.Update) user = User.query.filter_by(id=user_id, active=True).first_or_404()