diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 0c5ab341..ef11ee62 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -10,6 +10,7 @@ from sqlalchemy import or_, func from .. import db +from ..sync.models import ProjectUser from ..sync.utils import get_user_agent, get_ip, get_device_id @@ -149,24 +150,10 @@ def inactivate(self) -> None: """Inactivate user account and remove explicitly shared projects as well clean references to created projects. User is then safe to be removed. """ - from ..sync.models import Project, ProjectAccess, AccessRequest, RequestStatus - - shared_projects = Project.query.filter( - or_( - Project.access.has(ProjectAccess.owners.contains([self.id])), - Project.access.has(ProjectAccess.writers.contains([self.id])), - Project.access.has(ProjectAccess.editors.contains([self.id])), - Project.access.has(ProjectAccess.readers.contains([self.id])), - ) - ).all() - - for p in shared_projects: - for key in ("owners", "writers", "editors", "readers"): - value = set(getattr(p.access, key)) - if self.id in value: - value.remove(self.id) - setattr(p.access, key, list(value)) - db.session.add(p) + from ..sync.models import AccessRequest, RequestStatus + + # remove explicit permissions + ProjectUser.query.filter(ProjectUser.user_id == self.id).delete() # decline all access requests for req in ( diff --git a/server/mergin/auth/schemas.py b/server/mergin/auth/schemas.py index bcebca5c..95cac54e 100644 --- a/server/mergin/auth/schemas.py +++ b/server/mergin/auth/schemas.py @@ -33,17 +33,16 @@ def get_disk_usage(self, obj): def _has_project(self, obj): # DEPRECATED functionality - kept for the backward-compatibility - from ..sync.models import Project, ProjectAccess + from ..sync.models import ProjectUser, Project ws = current_app.ws_handler.get_by_name(obj.user.username) if ws: projects_count = ( - Project.query.filter(Project.creator_id == obj.user.id) + Project.query.join(ProjectUser) + .filter(Project.creator_id == obj.user.id) .filter(Project.removed_at.is_(None)) - .filter_by(workspace_id=ws.id) - .filter( - Project.access.has(ProjectAccess.readers.contains([obj.user.id])) - ) + .filter(Project.workspace_id == ws.id) + .filter(ProjectUser.user_id == obj.user.id) .count() ) return projects_count > 0 diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index a07d5966..68b3175f 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -11,7 +11,7 @@ from .files import UploadChanges from .. import db -from .models import Project, ProjectAccess, ProjectVersion +from .models import Project, ProjectVersion from .utils import split_project_path @@ -40,7 +40,6 @@ def create(name, namespace, user): # pylint: disable=W0612 p = Project(**project_params) p.updated = datetime.utcnow() db.session.add(p) - p.access = ProjectAccess(p, public=False) changes = UploadChanges(added=[], updated=[], removed=[]) pv = ProjectVersion(p, 0, user.id, changes, "127.0.0.1") pv.project = p diff --git a/server/mergin/sync/models.py b/server/mergin/sync/models.py index 83febbbd..702fe326 100644 --- a/server/mergin/sync/models.py +++ b/server/mergin/sync/models.py @@ -67,11 +67,16 @@ class Project(db.Model): removed_by = db.Column( db.Integer, db.ForeignKey("user.id"), nullable=True, index=True ) + public = db.Column(db.Boolean, default=False, index=True) creator = db.relationship( "User", uselist=False, backref=db.backref("projects"), foreign_keys=[creator_id] ) + project_users = db.relationship( + "ProjectUser", cascade="all, delete-orphan", back_populates="project" + ) + __table_args__ = (db.UniqueConstraint("name", "workspace_id"),) def __init__( @@ -82,6 +87,7 @@ def __init__( self.workspace_id = workspace.id self.creator = creator self.latest_version = 0 + self.public = kwargs.get("public", False) latest_files = LatestProjectFiles(project=self) db.session.add(latest_files) @@ -240,9 +246,7 @@ def delete(self, removed_by: int = None): db.session.execute( upload_table.delete().where(upload_table.c.project_id == self.id) ) - self.access.owners = self.access.writers = self.access.editors = ( - self.access.readers - ) = [] + self.project_users.clear() access_requests = ( AccessRequest.query.filter_by(project_id=self.id) .filter(AccessRequest.status.is_(None)) @@ -253,24 +257,43 @@ def delete(self, removed_by: int = None): db.session.commit() project_deleted.send(self) + def _member(self, user_id: int) -> Optional[ProjectUser]: + """Return association object for user_id""" + return next((u for u in self.project_users if u.user_id == user_id), None) + + def get_role(self, user_id: int) -> Optional[ProjectRole]: + """Get user role""" + member = self._member(user_id) + if member: + return ProjectRole(member.role) + + def set_role(self, user_id: int, role: ProjectRole) -> None: + """Set user role""" + member = self._member(user_id) + if member: + member.role = role.value + else: + self.project_users.append(ProjectUser(user_id=user_id, role=role.value)) + + def unset_role(self, user_id: int) -> None: + """Remove user's role""" + member = self._member(user_id) + if member: + self.project_users.remove(member) + class ProjectRole(Enum): - OWNER = "owner" - WRITER = "writer" - EDITOR = "editor" - READER = "reader" + """Project roles ordered by rank (do not change)""" - def __gt__(self, other): - """ - Compare project roles + READER = "reader" + EDITOR = "editor" + WRITER = "writer" + OWNER = "owner" - https://docs.python.org/3/library/enum.html#enum.EnumType.__members__ - """ + def __ge__(self, other): + """Compare project roles""" members = list(ProjectRole.__members__) - if members.index(self.name) < members.index(other.name): - return True - else: - return False + return members.index(self.name) >= members.index(other.name) @dataclass @@ -284,113 +307,6 @@ class ProjectAccessDetail: type: str -class ProjectAccess(db.Model): - project_id = db.Column( - UUID(as_uuid=True), - db.ForeignKey("project.id", ondelete="CASCADE"), - primary_key=True, - index=True, - ) - public = db.Column(db.Boolean, default=False, index=True) - owners = db.Column(ARRAY(db.Integer), server_default="{}") - readers = db.Column(ARRAY(db.Integer), server_default="{}") - writers = db.Column(ARRAY(db.Integer), server_default="{}") - editors = db.Column(ARRAY(db.Integer), server_default="{}") - - project = db.relationship( - "Project", - uselist=False, - backref=db.backref( - "access", - single_parent=True, - uselist=False, - cascade="all,delete", - lazy="joined", - ), - ) - - __table_args__ = ( - db.Index("ix_project_access_owners", owners, postgresql_using="gin"), - db.Index("ix_project_access_readers", readers, postgresql_using="gin"), - db.Index("ix_project_access_writers", writers, postgresql_using="gin"), - db.Index("ix_project_access_editors", editors, postgresql_using="gin"), - ) - - def __init__(self, project, public=False): - self.project = project - self.owners = [project.creator.id] - self.writers = [project.creator.id] - self.readers = [project.creator.id] - self.editors = [project.creator.id] - self.project_id = project.id - self.public = public - - def get_role(self, user_id: int) -> Optional[ProjectRole]: - """Get user role based on mapping to DB ACL""" - if user_id in self.owners: - return ProjectRole.OWNER - elif user_id in self.writers: - return ProjectRole.WRITER - elif user_id in self.editors: - return ProjectRole.EDITOR - elif user_id in self.readers: - return ProjectRole.READER - else: - return None - - @staticmethod - def _permission_attrs(role: ProjectRole) -> List[str]: - """Return db attributes list related to permission""" - # because roles do not inherit, they must be un/set explicitly in db ACLs - perm_list = { - ProjectRole.READER: ["readers"], - ProjectRole.EDITOR: ["editors", "readers"], - ProjectRole.WRITER: ["writers", "editors", "readers"], - ProjectRole.OWNER: ["owners", "writers", "editors", "readers"], - } - return perm_list[role] - - def set_role(self, user_id: int, role: ProjectRole) -> None: - """Set user role""" - self.unset_role(user_id) - for attr in self._permission_attrs(role): - ids = getattr(self, attr) - if user_id not in ids: - ids.append(user_id) - setattr(self, attr, ids) - flag_modified(self, attr) - - def unset_role(self, user_id: int) -> None: - """Remove user's role""" - role = self.get_role(user_id) - if not role: - return - - for attr in self._permission_attrs(role): - ids = getattr(self, attr) - if user_id in ids: - ids.remove(user_id) - setattr(self, attr, ids) - flag_modified(self, attr) - - def bulk_update(self, new_access: Dict) -> Set[int]: - """From new access lists do bulk update and return ids with any change applied""" - diff = set() - for key in ("owners", "writers", "editors", "readers"): - new_value = new_access.get(key, None) - if not new_value: - continue - old_value = set(getattr(self, key)) - diff = diff.union(set(new_value).symmetric_difference(old_value)) - setattr(self, key, list(new_value)) - - # make sure lists are consistent (they inherit from each other) - self.writers = list(set(self.writers).union(set(self.owners))) - self.editors = list(set(self.editors).union(set(self.writers))) - self.readers = list(set(self.readers).union(set(self.editors))) - return diff - - class ProjectFilePath(db.Model): """Files (paths) within Project""" @@ -1112,7 +1028,6 @@ def expire(self): def accept(self, permissions): """Accept project access request""" - project_access = self.project.access PERMISSION_PROJECT_ROLE = { "read": ProjectRole.READER, "edit": ProjectRole.EDITOR, @@ -1120,7 +1035,7 @@ def accept(self, permissions): "owner": ProjectRole.OWNER, } - project_access.set_role( + self.project.set_role( self.requested_by, PERMISSION_PROJECT_ROLE.get(permissions) ) self.resolve(RequestStatus.ACCEPTED, current_user.id) @@ -1205,3 +1120,29 @@ def __init__( if os.path.exists(diff_path): self.diff_size = os.path.getsize(diff_path) self.changes = GeoDiff().changes_count(diff_path) + + +class ProjectUser(db.Model): + """Association table for project membership""" + + __tablename__ = "project_member" + + project_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey("project.id", ondelete="CASCADE"), + primary_key=True, + ) + user_id = db.Column( + db.Integer, + db.ForeignKey("user.id", ondelete="CASCADE"), + primary_key=True, + ) + role = db.Column( + ENUM( + *[member.value for member in ProjectRole.__members__.values()], + name="project_role", + ), + nullable=False, + ) + project = db.relationship("Project", back_populates="project_users") + user = db.relationship("User") diff --git a/server/mergin/sync/permissions.py b/server/mergin/sync/permissions.py index 9ec1f981..d28ad48b 100644 --- a/server/mergin/sync/permissions.py +++ b/server/mergin/sync/permissions.py @@ -11,7 +11,7 @@ from .utils import is_valid_uuid from ..auth.models import User -from .models import ProjectAccess, Project, Upload, ProjectRole +from .models import Project, Upload, ProjectRole, ProjectUser def _is_superuser(f): @@ -48,11 +48,12 @@ class Read(Base): @_is_superuser def check(cls, project, user): # public active projects can be access by anyone - if project.access.public and not project.removed_at: + if project.public and not project.removed_at: return True + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( - (user.id in project.access.readers) + (project_role and project_role >= ProjectRole.READER) or (check_project_workspace_permissions(project, user, "read")) ) @@ -62,7 +63,7 @@ def query(cls, user, as_admin=True, public=True): return Project.query query = ( - Project.query.join(ProjectAccess) + Project.query.join(ProjectUser) .filter(Project.storage_params.isnot(None)) .filter(Project.removed_at.is_(None)) ) @@ -78,20 +79,20 @@ def query(cls, user, as_admin=True, public=True): if public: query = query.filter( or_( - ProjectAccess.public.is_(True), + Project.public.is_(True), Project.workspace_id.in_(user_workspace_ids), - ProjectAccess.readers.contains([user.id]), + ProjectUser.user_id == user.id, ) ) else: query = query.filter( or_( Project.workspace_id.in_(user_workspace_ids), - ProjectAccess.readers.contains([user.id]), + ProjectUser.user_id == user.id, ) ) else: - query = query.filter(ProjectAccess.public.is_(True)) + query = query.filter(Project.public.is_(True)) return query @@ -99,9 +100,10 @@ class Edit(Base): @classmethod @_is_superuser def check(self, project, user): + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( ( - user.id in project.access.editors + (project_role and project_role >= ProjectRole.EDITOR) or check_project_workspace_permissions(project, user, "edit") ) ) @@ -110,9 +112,10 @@ class Upload(Base): @classmethod @_is_superuser def check(cls, project, user): + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( ( - user.id in project.access.writers + (project_role and project_role >= ProjectRole.WRITER) or check_project_workspace_permissions(project, user, "write") ) ) @@ -121,9 +124,10 @@ class Update(Base): @classmethod @_is_superuser def check(cls, project, user): + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( ( - user.id in project.access.owners + project_role is ProjectRole.OWNER or check_project_workspace_permissions(project, user, "admin") ) ) @@ -132,9 +136,10 @@ class Delete(Base): @classmethod @_is_superuser def check(cls, project, user): + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( ( - user.id in project.access.owners + project_role is ProjectRole.OWNER or check_project_workspace_permissions(project, user, "admin") ) ) @@ -143,9 +148,10 @@ class All(Base): @classmethod @_is_superuser def check(cls, project, user): + project_role = project.get_role(user.id) if user.is_authenticated else None return super().check(project, user) and ( ( - user.id in project.access.owners + project_role is ProjectRole.OWNER or check_project_workspace_permissions(project, user, "admin") ) ) diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index c6122166..420f1c68 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -12,7 +12,14 @@ from ..auth import auth_required from ..auth.models import User, UserProfile from .forms import AccessPermissionForm -from .models import Project, AccessRequest, ProjectRole, RequestStatus, ProjectVersion +from .models import ( + Project, + AccessRequest, + ProjectRole, + RequestStatus, + ProjectVersion, + ProjectUser, +) from .schemas import ( ProjectListSchema, ProjectAccessRequestSchema, @@ -41,7 +48,7 @@ def create_project_access_request(namespace, project_name): # noqa: E501 Project.workspace_id == workspace.id, Project.removed_at.is_(None), ).first_or_404() - if current_user.id in project.access.readers: + if project.get_role(current_user.id): abort(409, "You already have access to project") access_request = ( @@ -59,9 +66,13 @@ def create_project_access_request(namespace, project_name): # noqa: E501 db.session.commit() # notify project owners owners = ( - User.query.join(UserProfile) - .filter(User.verified_email, User.id.in_(project.access.owners)) - .filter(UserProfile.receive_notifications) + User.query.join(UserProfile, ProjectUser) + .filter( + ProjectUser.project_id == project.id, + ProjectUser.role == ProjectRole.OWNER.value, + User.verified_email, + UserProfile.receive_notifications, + ) .all() ) for owner in owners: @@ -280,7 +291,7 @@ def unsubscribe_project(id): # pylint: disable=W0612 from ..celery import send_email_async project = require_project_by_uuid(id, ProjectPermissions.Read) - current_role = project.access.get_role(current_user.id) + current_role = project.get_role(current_user.id) if not current_role: return NoContent, 200 # nothing to do so request is idempotent @@ -288,7 +299,7 @@ def unsubscribe_project(id): # pylint: disable=W0612 if current_role == ProjectRole.OWNER: abort(400, "Owner cannot leave a project") - project.access.unset_role(current_user.id) + project.unset_role(current_user.id) db.session.add(project) db.session.commit() return NoContent, 200 @@ -303,7 +314,7 @@ def update_project_access(id: str): project = require_project_by_uuid(id, ProjectPermissions.Update) if "public" in request.json: - project.access.public = request.json["public"] + project.public = request.json["public"] if "user_id" in request.json and "role" in request.json: user = User.query.filter_by( @@ -311,12 +322,12 @@ def update_project_access(id: str): ).first_or_404("User does not exist") if request.json["role"] == "none": - project.access.unset_role(user.id) + project.unset_role(user.id) else: - project.access.set_role(user.id, ProjectRole(request.json["role"])) + project.set_role(user.id, ProjectRole(request.json["role"])) project_access_granted.send(project, user_id=user.id) db.session.commit() - return ProjectAccessSchema().dump(project.access), 200 + return ProjectAccessSchema().dump(project), 200 @auth_required diff --git a/server/mergin/sync/public_api_controller.py b/server/mergin/sync/public_api_controller.py index 7a3b1833..c3fb4bca 100644 --- a/server/mergin/sync/public_api_controller.py +++ b/server/mergin/sync/public_api_controller.py @@ -37,7 +37,6 @@ from ..auth.models import User from .models import ( Project, - ProjectAccess, ProjectVersion, Upload, PushChangeType, @@ -193,9 +192,13 @@ def add_project(namespace): # noqa: E501 "location": generate_location(), } - p = Project(**request.json, creator=current_user, workspace=workspace) + p = Project( + **request.json, + creator=current_user, + workspace=workspace, + public=request.json.get("public", False), + ) p.updated = datetime.utcnow() - pa = ProjectAccess(p, public=request.json.get("public", False)) template_name = request.json.get("template", None) if template_name: @@ -231,7 +234,6 @@ def add_project(namespace): # noqa: E501 ) db.session.add(p) - db.session.add(pa) db.session.add(version) db.session.commit() project_version_created.send(version) @@ -498,13 +500,15 @@ def get_projects_by_names(): # noqa: E501 Project.workspace_id == workspace.id, Project.name == name ).first() if result: - user_ids = ( - result.access.owners + result.access.writers + result.access.readers - ) - users_map = { - u.id: u.username - for u in User.query.filter(User.id.in_(set(user_ids))).all() - } + # FIXME + # user_ids = ( + # result.access.owners + result.access.writers + result.access.readers + # ) + # users_map = { + # u.id: u.username + # for u in User.query.filter(User.id.in_(set(user_ids))).all() + # } + users_map = None workspaces_map = {workspace.id: workspace.name} ctx = {"users_map": users_map, "workspaces_map": workspaces_map} results[project] = ProjectListSchema(context=ctx).dump(result) @@ -538,7 +542,8 @@ def get_projects_by_uuids(uuids): # noqa: E501 .all() ) for p in projects: - user_ids.extend(p.access.owners + p.access.writers + p.access.readers) + # FIXME + # user_ids.extend(p.access.owners + p.access.writers + p.access.readers) ws_ids.append(p.workspace_id) users_map = { u.id: u.username for u in User.query.filter(User.id.in_(set(user_ids))).all() @@ -621,7 +626,9 @@ def get_paginated_projects( # create user map id:username passed to project schema to minimize queries to db user_ids = [] for p in result: - user_ids.extend(p.access.owners + p.access.writers + p.access.readers) + # FIXME + # user_ids.extend(p.access.owners + p.access.writers + p.access.readers) + pass users_map = { u.id: u.username for u in User.query.filter(User.id.in_(set(user_ids))).all() @@ -660,7 +667,7 @@ def update_project(namespace, project_name): # noqa: E501 # pylint: disable=W0 return jsonify(error.to_dict()), 422 if "public" in request.json["access"]: - project.access.public = request.json["access"]["public"] + project.public = request.json["access"]["public"] db.session.add(project) db.session.commit() @@ -1186,7 +1193,6 @@ def clone_project(namespace, project_name): # noqa: E501 workspace=ws, ) p.updated = datetime.utcnow() - pa = ProjectAccess(p, public=False) try: p.storage.initialize(template_project=cloned_project) @@ -1212,7 +1218,6 @@ def clone_project(namespace, project_name): # noqa: E501 device_id, ) db.session.add(p) - db.session.add(pa) db.session.add(project_version) db.session.commit() project_version_created.send(project_version) diff --git a/server/mergin/sync/schemas.py b/server/mergin/sync/schemas.py index 827391a9..8cb425ef 100644 --- a/server/mergin/sync/schemas.py +++ b/server/mergin/sync/schemas.py @@ -126,7 +126,7 @@ def _uploads(self, obj): return [u.id for u in obj.project.uploads.all()] def _access(self, obj): - return ProjectAccessSchema().dump(obj.project.access) + return ProjectAccessSchema().dump(obj.project) def _permissions(self, obj): return project_user_permissions(obj.project) diff --git a/server/mergin/sync/workspace.py b/server/mergin/sync/workspace.py index 86360fef..bed55abc 100644 --- a/server/mergin/sync/workspace.py +++ b/server/mergin/sync/workspace.py @@ -9,7 +9,7 @@ from sqlalchemy.orm import joinedload from .errors import UpdateProjectAccessError -from .models import Project, ProjectAccess, AccessRequest, ProjectAccessDetail +from .models import Project, AccessRequest, ProjectAccessDetail, ProjectUser from .permissions import projects_query, ProjectPermissions from .public_api_controller import parse_project_access_update_request from .. import db @@ -160,10 +160,10 @@ def filter_projects( ): if only_public: projects = ( - Project.query.join(ProjectAccess) + Project.query.join(ProjectUser) .filter(Project.storage_params.isnot(None)) .filter(Project.removed_at.is_(None)) - .filter(ProjectAccess.public.is_(True)) + .filter(Project.public.is_(True)) ) else: projects = projects_query( @@ -185,7 +185,7 @@ def filter_projects( projects = projects.filter( or_( and_( - ProjectAccess.readers.contains([user.id]), + ProjectUser.user_id == user.id, Project.creator_id != user.id, ), and_( @@ -267,7 +267,9 @@ def update_project_members( """Update project members doing bulk access update""" error = None parsed_access = parse_project_access_update_request(access) - id_diffs = project.access.bulk_update(parsed_access) + # FIXME + id_diffs = set() + # id_diffs = project.access.bulk_update(parsed_access) db.session.add(project) db.session.commit() if parsed_access.get("invalid_usernames") or parsed_access.get("invalid_ids"): @@ -295,12 +297,7 @@ def project_access(self, project: Project) -> List[ProjectAccessDetail]: elif Configuration.GLOBAL_READ: global_role = "reader" - direct_members_ids = set( - project.access.readers - + project.access.editors - + project.access.writers - + project.access.owners - ) + direct_members_ids = [u.user_id for u in project.project_users] users = User.query.filter(User.active.is_(True)).order_by(User.email) direct_members = users.filter(User.id.in_(direct_members_ids)).all() diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index c4a12103..d092a361 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -13,7 +13,7 @@ from ..auth.models import User, UserProfile, LoginHistory from ..auth.tasks import anonymize_removed_users from .. import db -from ..sync.models import Project +from ..sync.models import Project, ProjectRole from . import ( test_workspace_id, json_headers, @@ -647,7 +647,7 @@ def test_close_user_account(client): user = User.query.filter_by(username=DEFAULT_USER[0]).first() # check default project project = Project.query.filter_by(workspace_id=test_workspace_id).first() - assert user.id in project.access.owners + assert project.get_role(user.id) is ProjectRole.OWNER resp = client.delete("/v1/user") assert resp.status_code == 204 @@ -657,7 +657,7 @@ def test_close_user_account(client): assert not user.active assert user.inactive_since assert ( - project.access.owners == [] + project.project_users == [] ) # project will be removed by cron job, but is not accessible # login should fail now diff --git a/server/mergin/tests/test_celery.py b/server/mergin/tests/test_celery.py index cc88e2d3..3bda23f1 100644 --- a/server/mergin/tests/test_celery.py +++ b/server/mergin/tests/test_celery.py @@ -126,7 +126,7 @@ def test_remove_deleted_project_backups(client): rm_project.removed_at and not rm_project.storage_params and not rm_project.files - and rm_project.access.owners == [] + and rm_project.project_users == [] ) assert ( not Project.query.filter_by(id=rm_project.id) diff --git a/server/mergin/tests/test_db_hooks.py b/server/mergin/tests/test_db_hooks.py index 0f16710d..50222c66 100644 --- a/server/mergin/tests/test_db_hooks.py +++ b/server/mergin/tests/test_db_hooks.py @@ -4,19 +4,19 @@ import os from pathlib import Path -from sqlalchemy.orm.attributes import flag_modified from ..sync.models import ( Project, ProjectVersion, Upload, - ProjectAccess, SyncFailuresHistory, AccessRequest, RequestStatus, FileHistory, ProjectFilePath, LatestProjectFiles, + ProjectRole, + ProjectUser, ) from ..sync.files import UploadChanges from ..auth.models import User @@ -38,8 +38,7 @@ def test_close_user_account(client, diff_project): user_id = user.id # user has access to mergin user diff_project - diff_project.access.writers.append(user.id) - flag_modified(diff_project.access, "writers") + diff_project.set_role(user.id, ProjectRole.WRITER) # user contributed to another user project so he is listed in projects history changes = UploadChanges(added=[], updated=[], removed=[]) pv = ProjectVersion(diff_project, 11, user.id, changes, "127.0.0.1") @@ -51,7 +50,7 @@ def test_close_user_account(client, diff_project): test_workspace = create_workspace() p = create_project(user_project, test_workspace, user) db.session.commit() - assert user.id in diff_project.access.writers + assert diff_project.get_role(user.id) is ProjectRole.WRITER proj_resp = client.get( f"/v1/project/{diff_project.workspace.name}/{diff_project.name}" ) @@ -67,7 +66,7 @@ def test_close_user_account(client, diff_project): # deactivate user first (direct hack in db to mimic inconsistency) user.active = False db.session.commit() - assert user.id in diff_project.access.writers # not yet removed + assert diff_project.get_role(user.id) is ProjectRole.WRITER # not yet removed proj_resp = client.get( f"/v1/project/{diff_project.workspace.name}/{diff_project.name}" ) @@ -96,7 +95,7 @@ def test_close_user_account(client, diff_project): ).count() == 1 ) - assert user_id not in diff_project.access.writers + assert not diff_project.get_role(user_id) # user remains referenced in existing project version he created (as read-only ref) assert diff_project.get_latest_version().author_id == user_id sync_fail_history = SyncFailuresHistory.query.filter( @@ -143,7 +142,7 @@ def test_remove_project(client, diff_project): assert Project.query.filter_by(id=project_id).count() assert not Upload.query.filter_by(project_id=project_id).count() assert ProjectVersion.query.filter_by(project_id=project_id).count() - assert ProjectAccess.query.filter_by(project_id=project_id).count() + assert not ProjectUser.query.filter_by(project_id=project_id).count() cleanup(client, [project_dir]) assert access_request.status == RequestStatus.DECLINED.value # after removal cached information in project table remains and project versions, but not files details diff --git a/server/mergin/tests/test_permissions.py b/server/mergin/tests/test_permissions.py index 0e986de3..76d25223 100644 --- a/server/mergin/tests/test_permissions.py +++ b/server/mergin/tests/test_permissions.py @@ -17,6 +17,7 @@ def test_project_permissions(client): owner = add_user("owner", "pwd") test_workspace = create_workspace() project = create_project("test_permissions", test_workspace, owner) + project.set_role(owner.id, ProjectRole.OWNER) # testing owner permissions -> has all of them assert ProjectPermissions.Read.check(project, owner) @@ -47,7 +48,7 @@ def test_project_permissions(client): # tests user with editor access -> has read access - project.access.set_role(user.id, ProjectRole.EDITOR) + project.set_role(user.id, ProjectRole.EDITOR) db.session.commit() assert ProjectPermissions.Read.check(project, user) assert ProjectPermissions.Edit.check(project, user) @@ -73,7 +74,7 @@ def test_project_permissions(client): # deactivate user -> no permissions user.active = False - project.access.unset_role(user.id) + project.unset_role(user.id) db.session.commit() assert not ProjectPermissions.Upload.check(project, user) assert not ProjectPermissions.Delete.check(project, user) @@ -86,7 +87,7 @@ def test_project_permissions(client): # tests anonymous user -> only has read access if project is public user = AnonymousUserMixin() assert not ProjectPermissions.Read.check(project, user) - project.access.public = True + project.public = True db.session.commit() assert ProjectPermissions.Read.check(project, user) assert not ProjectPermissions.Edit.check(project, user) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 9bd66b7b..4e61a88c 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -20,7 +20,7 @@ def test_project_unsubscribe(client, diff_project): # create user and grant him write access user = add_user("reader", "reader") - diff_project.access.set_role(user.id, ProjectRole.WRITER) + diff_project.set_role(user.id, ProjectRole.WRITER) db.session.commit() # project owner is logged in @@ -42,9 +42,7 @@ def test_project_unsubscribe(client, diff_project): ) ) assert resp.status_code == 200 - assert not diff_project.access.get_role(user.id) - assert user.id not in diff_project.access.readers - assert user.id not in diff_project.access.writers + assert not diff_project.get_role(user.id) def test_project_access_request(client): @@ -151,9 +149,7 @@ def test_project_access_request(client): project = Project.query.filter( Project.name == "testx", Project.workspace_id == test_workspace.id ).first() - assert user2.id in project.access.readers - assert user2.id in project.access.writers - assert user2.id not in project.access.owners + assert project.get_role(user2.id) is ProjectRole.WRITER # no request listed resp = client.get( @@ -337,52 +333,49 @@ def test_update_project_access(client, diff_project): original_creator_id = diff_project.creator.id # create user and grant him write access user = add_user("reader", "reader") - assert user.id not in diff_project.access.readers + assert not diff_project.get_role(user.id) data = {"user_id": user.id, "role": "none"} # nothing happens resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id not in diff_project.access.readers + assert not diff_project.get_role(user.id) # grant read access data["role"] = "reader" resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id in diff_project.access.readers + assert diff_project.get_role(user.id) is ProjectRole.READER # grant editor access data["role"] = "editor" resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id in diff_project.access.editors + assert diff_project.get_role(user.id) is ProjectRole.EDITOR # change to write access data["role"] = "writer" resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id in diff_project.access.readers - assert user.id in diff_project.access.writers + assert diff_project.get_role(user.id) is ProjectRole.WRITER # downgrade to read access data["role"] = "reader" resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id in diff_project.access.readers - assert user.id not in diff_project.access.writers + assert diff_project.get_role(user.id) is ProjectRole.READER # remove access data["role"] = "none" resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert user.id not in diff_project.access.readers - assert user.id not in diff_project.access.writers + assert not diff_project.get_role(user.id) # update public parameter => public: True data["public"] = True resp = client.patch(url, headers=json_headers, data=json.dumps(data)) assert resp.status_code == 200 - assert diff_project.access.public == True + assert diff_project.public == True # access of project creator can be removed data["user_id"] = diff_project.creator_id @@ -393,8 +386,7 @@ def test_update_project_access(client, diff_project): ) assert resp.status_code == 200 db.session.rollback() - assert user.id not in diff_project.access.owners - assert user.id not in diff_project.access.readers + assert not diff_project.get_role(user.id) assert diff_project.creator_id == original_creator_id # try to grant access to inaccessible user @@ -495,10 +487,9 @@ def test_get_project_access(client): assert resp.status_code == 200 assert len(resp.json) == 1 assert resp.json[0]["project_permission"] == "owner" - project.access.set_role(users[0].id, ProjectRole.OWNER) - project.access.set_role(users[1].id, ProjectRole.WRITER) - project.access.set_role(users[2].id, ProjectRole.READER) - db.session.add(project.access) + project.set_role(users[0].id, ProjectRole.OWNER) + project.set_role(users[1].id, ProjectRole.WRITER) + project.set_role(users[2].id, ProjectRole.READER) db.session.commit() resp = client.get(url) assert resp.status_code == 200 diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py index 216f4b48..8bfa4141 100644 --- a/server/mergin/tests/test_project_controller.py +++ b/server/mergin/tests/test_project_controller.py @@ -28,7 +28,6 @@ Project, Upload, ProjectVersion, - ProjectAccess, SyncFailuresHistory, GeodiffActionHistory, ProjectRole, @@ -288,7 +287,7 @@ def test_get_paginated_projects(client): assert resp_data.get("count") == 1 assert not resp_data.get("projects")[0].get("has_conflict") - project.access.public = True + project.public = True db.session.commit() # reset permissions so new user would be only a guest @@ -308,7 +307,7 @@ def test_get_paginated_projects(client): assert resp.json["count"] == 0 # share project explicitly p = Project.query.filter_by(name="foo1").first() - p.access.set_role(user2.id, ProjectRole.READER) + p.set_role(user2.id, ProjectRole.READER) db.session.commit() resp = client.get("/v1/project/paginated?page=1&per_page=10&flag=shared") assert resp.json["count"] == 1 @@ -628,7 +627,7 @@ def test_update_project(client): name=test_project, workspace_id=test_workspace_id ).first() # need for private project - project.access.public = False + project.public = False db.session.add(project) # add some tester test_user = User( @@ -640,18 +639,30 @@ def test_update_project(client): db.session.commit() # add tests user as reader to project - data = {"access": {"readers": project.access.readers + [test_user.id]}} + data = { + "access": { + "readers": [ + u.id + for u in project.project_users + if u.role == ProjectRole.READER.value + ] + + [test_user.id] + } + } resp = client.put( "/v1/project/{}/{}".format(test_workspace_name, test_project), data=json.dumps(data), headers=json_headers, ) assert resp.status_code == 200 - assert test_user.id in project.access.readers + assert project.get_role(test_user.id) is ProjectRole.READER # add tests user as writer to project + current_writers = [ + u.id for u in project.project_users if u.role == ProjectRole.WRITER.value + ] writers = [ - u.username for u in User.query.filter(User.id.in_(project.access.writers)).all() + u.username for u in User.query.filter(User.id.in_(current_writers)).all() ] data = {"access": {"writersnames": writers + [test_user.username]}} resp = client.put( @@ -660,7 +671,7 @@ def test_update_project(client): headers=json_headers, ) assert resp.status_code == 200 - assert test_user.id in project.access.writers + assert project.get_role(test_user.id) is ProjectRole.WRITER # try to remove project creator from owners data = {"access": {"owners": [test_user.id]}} @@ -672,9 +683,11 @@ def test_update_project(client): assert resp.status_code == 200 # try to add non-existing user + current_readers = [ + u.id for u in project.project_users if u.role == ProjectRole.READER.value + ] readers = [ - user.username - for user in User.query.filter(User.id.in_(project.access.readers)).all() + user.username for user in User.query.filter(User.id.in_(current_readers)).all() ] data = {"access": {"readersnames": readers + ["not-found-user"]}} resp = client.put( @@ -688,9 +701,11 @@ def test_update_project(client): assert resp.json["invalid_usernames"] == ["not-found-user"] # try to add non-existing user plus make some valid update -> only partial success + current_readers = [ + u.id for u in project.project_users if u.role == ProjectRole.READER.value + ] readers = [ - user.username - for user in User.query.filter(User.id.in_(project.access.readers)).all() + user.username for user in User.query.filter(User.id.in_(current_readers)).all() ] data = { "access": { @@ -1129,10 +1144,10 @@ def test_push_to_new_project(client): p = Project.query.filter_by( name=test_project, workspace_id=test_workspace_id ).first() - project = Project("blank", p.storage_params, p.creator, p.workspace, files=[]) + project = Project( + "blank", p.storage_params, p.creator, p.workspace, files=[], public=True + ) db.session.add(project) - pa = ProjectAccess(project, True) - db.session.add(pa) db.session.commit() current_app.config["BLACKLIST"] = ["test4"] @@ -1431,7 +1446,7 @@ def test_push_finish(client): project = Project.query.filter_by( name=test_project, workspace_id=test_workspace_id ).first() - project.access.owners.append(user.id) + project.set_role(user.id, ProjectRole.OWNER) db.session.commit() upload, upload_dir = create_transaction(user.username, changes) @@ -1775,7 +1790,7 @@ def test_clone_project(client, data, username, expected): assert os.path.exists( os.path.join(project.storage.project_dir, project.files[0].location) ) - assert not project.access.public + assert not project.public # check if there is no diffs in cloned files assert not any(file.diff for file in project.files) pv = project.get_latest_version() @@ -2111,7 +2126,7 @@ def test_orphan_project(client): test_workspace = create_workspace() project = create_project("orphan", test_workspace, user) assert project.creator_id == user_id - assert project.access.owners == [user_id] + assert project.get_role(user_id) is ProjectRole.OWNER # user is removed by superuser login_as_admin(client) @@ -2124,7 +2139,7 @@ def test_orphan_project(client): # project still exists (it belongs to workspace) p = Project.query.filter_by(name="orphan").first() assert p.creator_id - assert p.access.owners == [] + assert len(p.project_users) == 0 # superuser as workspace owner has access to project and can assign new writer/owner resp = client.get(f"/v1/project/{test_workspace.name}/{p.name}") @@ -2169,7 +2184,7 @@ def test_orphan_project(client): def test_inactive_project(client, diff_project): """Project set for removal is not listed and can not be updated""" user = add_user("tests", "tests") - diff_project.access.set_role(user.id, ProjectRole.OWNER) + diff_project.set_role(user.id, ProjectRole.OWNER) diff_project.removed_at = datetime.datetime.utcnow() db.session.commit() project_path = get_project_path(diff_project) @@ -2208,7 +2223,7 @@ def test_inactive_project(client, diff_project): assert resp.status_code == 404 # modify project - data = {"access": {"readers": diff_project.access.readers + [user.id]}} + data = {"access": {"readers": [user.id]}} resp = client.put( f"/v1/project/{project_path}", data=json.dumps(data), headers=json_headers ) diff --git a/server/mergin/tests/utils.py b/server/mergin/tests/utils.py index 121acf2f..3aeaf445 100644 --- a/server/mergin/tests/utils.py +++ b/server/mergin/tests/utils.py @@ -18,7 +18,7 @@ from ..auth.models import User, UserProfile from ..sync.utils import generate_location, generate_checksum -from ..sync.models import Project, ProjectAccess, ProjectVersion, FileHistory +from ..sync.models import Project, ProjectVersion, FileHistory, ProjectRole from ..sync.files import UploadChanges, ChangesSchema from ..sync.workspace import GlobalWorkspace from .. import db @@ -79,11 +79,8 @@ def create_project(name, workspace, user, **kwargs): p = Project(**project_params, **kwargs) p.updated = datetime.utcnow() + p.set_role(user.id, ProjectRole.OWNER) db.session.add(p) - - public = kwargs.get("public", False) - pa = ProjectAccess(p, public) - db.session.add(pa) changes = UploadChanges(added=[], updated=[], removed=[]) pv = ProjectVersion(p, 0, user.id, changes, "127.0.0.1") db.session.add(pv) @@ -169,12 +166,10 @@ def initialize(): } ) p.latest_version = 1 + p.public = True + p.set_role(user.id, ProjectRole.OWNER) p.updated = datetime.utcnow() db.session.add(p) - - # add default project permissions - pa = ProjectAccess(p, True) - db.session.add(pa) db.session.commit() upload_changes = ChangesSchema(context={"version": 1}).load( diff --git a/server/migrations/community/d02961c7416c_add_project_member_table.py b/server/migrations/community/d02961c7416c_add_project_member_table.py new file mode 100644 index 00000000..526167d0 --- /dev/null +++ b/server/migrations/community/d02961c7416c_add_project_member_table.py @@ -0,0 +1,231 @@ +"""Add project member table + +Revision ID: d02961c7416c +Revises: 1ab5b02ce532 +Create Date: 2024-10-31 15:20:52.833051 + +""" + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "d02961c7416c" +down_revision = "1ab5b02ce532" +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table( + "project_member", + sa.Column("project_id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("user_id", sa.Integer(), nullable=False), + sa.Column( + "role", + postgresql.ENUM("reader", "editor", "writer", "owner", name="project_role"), + nullable=False, + ), + sa.ForeignKeyConstraint( + ["project_id"], + ["project.id"], + name=op.f("fk_project_member_project_id_project"), + ondelete="CASCADE", + ), + sa.ForeignKeyConstraint( + ["user_id"], + ["user.id"], + name=op.f("fk_project_member_user_id_user"), + ondelete="CASCADE", + ), + sa.PrimaryKeyConstraint( + "project_id", "user_id", name=op.f("pk_project_member") + ), + ) + + op.add_column("project", sa.Column("public", sa.Boolean(), nullable=True)) + op.create_index(op.f("ix_project_public"), "project", ["public"], unique=False) + + data_upgrade() + + op.drop_table("project_access") + + +def downgrade(): + op.create_table( + "project_access", + sa.Column("public", sa.BOOLEAN(), autoincrement=False, nullable=True), + sa.Column( + "owners", + postgresql.ARRAY(sa.INTEGER()), + server_default=sa.text("'{}'::integer[]"), + autoincrement=False, + nullable=True, + ), + sa.Column( + "readers", + postgresql.ARRAY(sa.INTEGER()), + server_default=sa.text("'{}'::integer[]"), + autoincrement=False, + nullable=True, + ), + sa.Column( + "writers", + postgresql.ARRAY(sa.INTEGER()), + server_default=sa.text("'{}'::integer[]"), + autoincrement=False, + nullable=True, + ), + sa.Column("project_id", postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column( + "editors", + postgresql.ARRAY(sa.INTEGER()), + server_default=sa.text("'{}'::integer[]"), + autoincrement=False, + nullable=True, + ), + sa.ForeignKeyConstraint( + ["project_id"], + ["project.id"], + name="fk_project_access_project_id_project", + ondelete="CASCADE", + ), + sa.PrimaryKeyConstraint("project_id", name="pk_project_access"), + ) + op.create_index( + "ix_project_access_writers", + "project_access", + ["writers"], + unique=False, + postgresql_using="gin", + ) + op.create_index( + "ix_project_access_readers", + "project_access", + ["readers"], + unique=False, + postgresql_using="gin", + ) + op.create_index( + "ix_project_access_public", "project_access", ["public"], unique=False + ) + op.create_index( + "ix_project_access_project_id", "project_access", ["project_id"], unique=False + ) + op.create_index( + "ix_project_access_owners", + "project_access", + ["owners"], + unique=False, + postgresql_using="gin", + ) + op.create_index( + "ix_project_access_editors", + "project_access", + ["editors"], + unique=False, + postgresql_using="gin", + ) + + data_downgrade() + + op.drop_index(op.f("ix_project_public"), table_name="project") + op.drop_column("project", "public") + op.drop_table("project_member") + conn = op.get_bind() + conn.execute(sa.text("DROP TYPE project_role;")) + + +def data_upgrade(): + conn = op.get_bind() + conn.execute( + sa.text( + """ + WITH members AS ( + SELECT + project_id AS project_id, + unnest(owners) AS user_id, + 'owner' AS role + FROM project_access + UNION + SELECT + project_id AS project_id, + unnest(writers - owners) AS user_id, + 'writer' AS role + FROM project_access + UNION + SELECT + project_id AS project_id, + unnest(editors - writers - owners) AS user_id, + 'editor' AS role + FROM project_access + UNION + SELECT + project_id AS project_id, + unnest(readers - editors - writers - owners) AS user_id, + 'reader' AS role + FROM project_access + ) + INSERT INTO project_member (project_id, user_id, role) + SELECT DISTINCT project_id, user_id, role::project_role FROM members; + """ + ) + ) + + conn.execute( + sa.text( + """ + UPDATE project p + SET + public = pa.public + FROM project_access pa + WHERE pa.project_id = p.id; + """ + ) + ) + + +def data_downgrade(): + conn = op.get_bind() + conn.execute( + sa.text( + """ + WITH members AS ( + WITH agg AS ( + SELECT + project_id, + array_agg(user_id) AS users_ids, + role + FROM + project_member + GROUP BY project_id, role + ) + SELECT + o.project_id, + o.users_ids AS owners, + o.users_ids || w.users_ids AS writers, + o.users_ids || w.users_ids || e.users_ids AS editors, + o.users_ids || w.users_ids || e.users_ids || r.users_ids AS readers + FROM (SELECT * FROM agg WHERE role = 'owner') AS o + LEFT OUTER JOIN (SELECT * FROM agg WHERE role = 'reader') AS r ON o.project_id = r.project_id + LEFT OUTER JOIN (SELECT * FROM agg WHERE role = 'editor') AS e ON o.project_id = e.project_id + LEFT OUTER JOIN (SELECT * FROM agg WHERE role = 'writer') AS w ON o.project_id = w.project_id + ) + INSERT INTO project_access (project_id, owners, writers, editors, readers) + SELECT DISTINCT project_id, owners, writers, editors, readers FROM members m; + """ + ) + ) + + conn.execute( + sa.text( + """ + UPDATE project_access pa + SET + public = p.public + FROM project p + WHERE pa.project_id = p.id; + """ + ) + )