feat(course-filters): tighten PATCH/POST contract (required name, for… - #178
Merged
Conversation
…bid extras)
- POST and PATCH bodies now reject unknown keys (extra="forbid") so a
typo or a stale frontend surfaces as 422 instead of being silently
dropped (Pydantic's default).
- PATCH.name is now REQUIRED, not Optional. The only editable field is
name, and a body without it is a no-op — accepting {} hid client bugs
that would only show up in prod. When a second editable field is added
later, relax this back to Optional + add an at-least-one-set validator.
- Service simplified: the unreachable empty-update branch is gone.
- Tests: +4 cases (extra-fields rejected on POST/PATCH, empty PATCH body
returns 422, same-name PATCH is a valid no-op that still hits update()).
DarkSerme
added a commit
that referenced
this pull request
Jul 24, 2026
* feat(deployments): per-group + admin SSH keys with download (#153) * feat(deployments): per-group + admin SSH keys with secure download endpoint - Generate Ed25519 keypairs per group (opt-in via app.yaml ssh_key: generate) and always for the teacher as admin SSH key - Rename per_student -> per_group in app.yaml credentials schema (hard cut); generator output key students -> groups - Persist ssh_private_key in deployment_instance_access (Fernet-encrypted via existing EncryptedString TypeDecorator; reuses the column already in DB) - Install teacher admin public key into authorized_keys via base.yml playbook (ansible.posix.authorized_key) - Expose ssh_private_key in GET /deployments/{id}/credentials response - Add GET /deployments/{id}/credentials/access/{access_id}/ssh-key download endpoint returning application/x-pem-file with Content-Disposition attachment - Make EncryptedString fail loudly on misconfig instead of silently storing plaintext / returning ciphertext as plaintext - Migrate APIResponse to ConfigDict (Pydantic V2 deprecation cleanup) Tests: +20 unit/api tests covering keypair generation, credential generator with new schema, persistence + decryption, ownership/project-scope enforcement (including 403 for non-owning lecturers), and admin bypass. * fix(mypy): annotate _build_credential_entry result as dict[str, Any] The dict can hold both plain strings (passwords / resolved templates) and nested dicts (ssh_key keypairs). Without the explicit annotation mypy infers the type from the first assignment (dict for ssh_key) and rejects the subsequent string assignments. * ci: remove prod GitHub App env sync step from deploy-production * fix(deployments): expose access.id in /credentials response (#155) The download endpoint /credentials/access/{access_id}/ssh-key takes access_id as a path parameter, but /credentials never returned it — making the download endpoint effectively unreachable from the frontend. Add the id field to DeploymentCredentialEntry so callers can build the download URL. Discovered via staging smoke-test on existing deployments. * feat(student): self-service endpoint for credentials (#156) * feat(student): self-service endpoint for credentials of own group's deployments Adds a STUDENT-only API surface so students can log in and fetch ONLY the credentials of deployments where they personally are a member of a group. They never see other groups' credentials, never see the teacher's admin credentials, never see any other deployment data. ## Three new endpoints (router-level require_roles(STUDENT)) - GET /api/v1/student/deployments Lists deployments where the caller is in at least one group that has credentials for that deployment. Returns a trimmed schema (StudentDeploymentResponse) — no deployment_parameters, no teacher info, no other students' personal data. - GET /api/v1/student/deployments/{id}/credentials Returns ONLY the access rows whose group_id matches one of the student's group memberships. Admin credentials (group_id IS NULL) and other groups' credentials are filtered out at the SQL layer. - GET /api/v1/student/deployments/{id}/credentials/access/{access_id}/ssh-key Same PEM-download flow as the lecturer endpoint, but the access row's group_id MUST be one of the student's groups. Adversarial access_id attempts (group B's id from a student of group A) get 403, not data. ## The missing link: DeploymentInstanceAccess.group_id The data model already had course_groups + group_members + course_members tables populated by the lecturer-facing courses API, but DeploymentInstance.group_id was nullable AND never populated. This change adds a group_id FK on DeploymentInstanceAccess (the right granularity, since one stack can host multiple groups' credential sets) and plumbs it through the wizard → deploy task → credential service chain: - src/schemas/deployment.py: optional course_group_id on GroupInfo - src/services/credential_generator_service.py: forward course_group_id - src/tasks/deploy_tasks.py: include group_id in credentials_for_db - src/services/deployment_credential_service.py: stamp it on access rows NULL group_id => teacher/admin credential => never returned to students. ## Backfill migration alembic/versions/b6d52b9f8ea3_backfill_access_group_id.py walks every existing deployment.deployment_parameters JSON, looks up matching course_groups by (course_id, name), and stamps group_id onto access rows whose sanitized username matches. Idempotent; gracefully skips deployments with no matching CourseGroup row (those access rows stay invisible to students; lecturer flow keeps working). ## Tests (+22 new tests, 358 total green) - tests/api/test_student_routes.py — 12 tests. Uses a real in-memory SQLite DB with the production models so the multi-join authorization query is exercised end-to-end, not mocked. Covers all security boundaries: own group visible, other group filtered, admin filtered, adversarial access_id rejected with 403 and no PEM in body. - tests/unit/test_backfill_access_group_id.py — 6 tests on the migration logic itself (in-memory SQLite + monkey-patched alembic bind). - tests/unit/test_credential_generator_service.py + test_deployment_credential_service.py — extended for course_group_id / group_id propagation. ## Out of scope (intentional) - Per-student-individual credentials (today is per-group): trivially extensible later via DeploymentInstanceAccess.course_member_id. - Frontend wizard: must start sending course_group_id per group on deployment creation. Until then, lecturer flow keeps working but new deployments don't enable student visibility — backfill is fallback for cases where lecturers used the courses API to persist groups. - Keycloak realm setup for the 'student' role is a one-off operations step; not in code. * fix(tests): unconditionally override ENCRYPTION_KEY with a valid Fernet key CI sets ENCRYPTION_KEY to a placeholder string ('dGVzdGtleXRlc3RrZXl0...') that decodes to 29 bytes — Fernet requires exactly 32. This was masked for years by EncryptedString silently falling back to plaintext on misconfig; now that we made it fail loudly (security fix in earlier PR), any test that touches an EncryptedString column blows up in CI. Switch from setdefault() to unconditional assignment so the conftest value wins regardless of what CI provides. The singleton in secret_encryption_service is lazy, so this override (in conftest module- import time) always lands before the first Fernet() call. Long-term cleanup: the CI workflow's env block should either be removed (redundant now) or fixed to a valid Fernet key, but that's a workflow change for a separate PR. * fix(template-version-files): enforce permission check on PATCH/DELETE (#157) PATCH /template-version-files/{file_id} and DELETE /template-version-files/{file_id} called update_file()/delete_file() WITHOUT forwarding the caller's user_id and admin flag. The internal _check_version_access then evaluated against user_id=None, is_admin=False — failing closed (403) for everyone, including admins and template owners. Effectively neither endpoint was usable from the API in any role. (Discovered when trying to retro-correct file_type for a GitHub-imported template whose subdir files came in as OTHER — the PATCH-by-API workaround was blocked.) Changes: - Service: update_file() and delete_file() take user_id + is_admin params and forward them to the per-version access check (admins always pass; otherwise template owner only — matches get_file()/get_version_files()). - API endpoints: pass current_user["user_id"] + is_admin from the CurrentUser dependency, same pattern the read endpoints in this router already use. Regression tests in tests/unit/test_template_access_control.py: - update_file forbidden without user_id (the original bug) - update_file forbidden for non-owner non-admin - update_file allowed for admin / for owner - delete_file forbidden for non-owner non-admin (and the repo's .delete() must NOT be invoked — guards against future refactors that move the delete before the permission check) - delete_file allowed for admin / for owner 365 tests green, ruff clean. * fix(mypy): normalise file_id to UUID before BaseRepository.delete (#158) BaseRepository.delete() is typed (id: UUID); update_file takes str | UUID to match the wider service convention. Forwarding the string straight through made mypy complain. Convert at the call site instead of narrowing the public signature. * fix(ansible): rename extra-var key 'groups' to 'deployment_groups' (#159) Ansible reserves ``groups`` as the inventory dict (mapping group names to host lists). Passing our per-group credential list as --extra-vars under that name silently loses to Ansible's built-in: every loop in the template playbook would iterate over the wrong thing — the inventory groups, not our deployment groups. The earlier hard-cut rename (students -> groups) collided exactly here. This fix changes only the extra-vars top-level key: - CredentialGeneratorService.generate(...) now returns {"deployment_groups": [...], "teacher": {...}} - deploy_tasks.py: read generated["deployment_groups"] in both places that build credentials_for_db / ssh_allow_users. - tests: assert "deployment_groups" in creds; also add a negative assertion that "groups" must NOT leak through. Schema-side stays unchanged: - credentials.per_group in app.yaml (semantic key, not an Ansible var) - StackAssignment.groups Pydantic field (HTTP request shape) - DeploymentInstanceAccess.group_id column (DB shape) App-side: every playbook that loops ``{{ groups }}`` must rename to ``{{ deployment_groups }}``. Reference playbook in app-repo (PR pending). Lecturer-side flow uses no ansible-only ``groups`` reference, unaffected. Discovered during end-to-end review of ansible_multiuser deployment; without this fix Ansible would silently iterate over the inventory dict instead of the credential list. * feat(app-manifest): accept lists for shell_scripts and config_files * feat(templates): approval flow applies only to public templates (#161) * feat(deployments): cooperative cancel for in-flight deploys via DELETE (#162) * feat(deployments): cooperative cancel for in-flight deploys via DELETE * fix(mypy): annotate created_stack_ids list type * fix(test): mock socket in wait_for_ssh test to avoid CI port-22 flakiness * test(cancellation): replace flaky socket-mock test with direct check * feat: credentials API exposes group_id+group_name; live SSE; seed rewrite * `api/deployments.py` * GET /credentials: response entries enthalten jetzt `group_id` und `group_name` (per JOIN auf course_groups). Damit kann das Frontend die Credentials nach Owner sortieren (Dozent fuer group_id=NULL, Gruppe X fuer den jeweiligen group_name). * GET /logs/stream: SSE-Endpoint umgebaut. - Frische SessionLocal pro Polling-Tick statt Request-Session — sonst sieht der Endpoint nie Logs, die der Celery-Worker committed hat, weil SQLAlchemy in derselben Transaktion cached. - run_in_threadpool um die sync DB-Aufrufe, damit die Loop andere SSE-Clients nicht blockt. - 15s-Heartbeat (`: ping`), damit Browser/Proxies waehrend langer Ansible-Phasen die Connection nicht fuer tot halten. - Connection: keep-alive Header. * `schemas/deployment.py`: `DeploymentCredentialEntry` um optional `group_id` + `group_name` erweitert. * `services/credential_generator_service.py`: `_sanitize_username` mappt jetzt auch Bindestriche auf Underscores — damit der generierte username (z.B. aus "Gruppe 1") das strikte Postgres-Identifier-Pattern `^[a-z_][a-z0-9_]{0,62}$` erfuellt, das ansible_postgres_group_db im Playbook validiert. Vorher scheiterte jeder Deploy mit Spaces/Punkten/Strichen im Gruppennamen mit "Ungueltige Gruppen-Credentials fuer PostgreSQL/pgAdmin". * `utils/app_manifest_parser.py`: parsed jetzt eine `ansible:` Sektion (mit `vars:` Mapping) aus der app.yaml. Schon vom Backend-Code vorgesehen aber im Parser nicht durchgereicht. * `core/seed_data.py`: komplett neu geschrieben. - 10 Datei-Inhalte direkt aus appstore-apps/ansible_multiuser/ und appstore-apps/ansible_postgres_group_db/ eingebettet (raw triple- single-quote strings, byte-identisch zur Disk). - Zwei Templates: "Multi-User Ubuntu", "PostgreSQL Group DB" — ohne "Ansible "-Praefix im Display-Namen. - Legacy-Template-Namen ("Ansible Multi-User Ubuntu", "PostgreSQL Group Database", "Ansible PostgreSQL Group DB") werden beim Seeder- Lauf geloescht — saubere DB nach Restart. - Idempotente Upsert-Logic: Templates/Versionen werden wiederverwendet wenn vorhanden, Files per `file_path` upserted. * `scripts/sync_app_files_to_db.py`: neues Helfer-Skript um die Files einer bereits gesseededten App-Version aus appstore-apps/<dir>/ in die DB zu schieben — fuer den dev-Workflow "Playbook editieren → testen" ohne Backend-Restart. * test: align fixtures + backfill migration with new sanitize behavior * `tests/api/test_deployment_credentials_routes.py`: `_build_access` setzt jetzt explizit group_id (default None) und group bzw group.name. Vorher lieferte MagicMock automatisch MagicMock-Objekte fuer beide Felder, was Pydantic mit "Input should be a valid string" abgelehnt hat, seit dem Schema die zwei Felder als str|None deklariert. * `alembic/versions/b6d52b9f8ea3_backfill_access_group_id.py`: inlined _sanitize_username an credential_generator_service._sanitize_username angeglichen (Bindestrich → Underscore). Sonst wuerde der Backfill nach "gruppe-1" suchen, der Service hat aber "gruppe_1" geschrieben — keine Matches. Bewusst eine bereits releaste Migration angefasst: Production-DBs die die Migration schon laufen liessen sind nicht betroffen (Alembic re-applied nicht). Neue DBs / lokale Re-Resets bekommen das korrekte Verhalten. * `tests/unit/test_backfill_access_group_id.py`: Fixtures auf "gruppe_1" geupdated, damit sie zum neuen Sanitize-Output passen. * feat(overleaf): persist activation links via post-Ansible SSH fetch Overleaf is the first app where credentials are not generated pre-Ansible (no password / SSH key per user) but rather as one-time activation URLs emitted by the Overleaf CLI during the playbook run. The playbook writes them to /opt/dozilab/OVERLEAF_USERS.json (mode 0600 root:root); without this change they live nowhere outside the VM. Wiring (generic, reusable for future apps that follow the same JSON shape): - AnsibleService: new fetch_remote_file / fetch_remote_json helpers that ssh+sudo cat a root-owned path off the VM. Never raises — missing file is logged as INFO and returns None so unrelated apps keep working. - AccessType.ACTIVATION_LINK added; new Alembic migration extends the postgres `accesstype` enum with the new value. - DeploymentCredentialService.persist_activation_links() takes the parsed JSON plus a username→course_groups.id map, writes one access row per admin + per group. Bypasses the pre-Ansible password/key filter on purpose. Unknown usernames are skipped with a warning rather than written with NULL group_id (which would leak to no student through the self-service filter). - deploy_tasks: after run_playbooks succeeds, attempt the fetch and the persist call. Wrapped in its own try/except — a fetch failure logs WARNING but never fails the deployment (the file remains on the VM for manual recovery). API/schema unchanged: access_type is already serialized as `.value` string, schemas don't validate the enum, student-self-service filters on group_id only — students automatically see their group's link, never the admin link. * fix(templates): cascade-delete versions when deleting a template (#166) Deleting a template returned a 500 because template_versions.template_id had no ON DELETE CASCADE — the FK constraint violation surfaced as a generic SQLAlchemyError swallowed by 'except Exception: raise' in the service and re-emitted by the generic exception handler. - Add ON DELETE CASCADE on template_versions.template_id and template_version_files.template_version_id, plus ORM cascade='all, delete-orphan' on the matching relationships. - New alembic migration d5e8c2a91b34 to drop+recreate the FKs. - Pre-check in TemplateService.delete_template: if any version still has deployments, return a 400 with a clear message instead of letting the deployments FK trip an opaque 500. - Drop the no-op try/except in delete_template that only re-raised. - Enable PRAGMA foreign_keys=ON for the SQLite test engine so cascade semantics match Postgres in tests. - Add tests for the cascade path and the deployment-blocked 400. * fix(alembic): chain cascade migration onto current head 034d40e1dad3 (#167) Was branched off the older a7c4f2b91d34 by mistake, causing 'Multiple head revisions are present' on alembic upgrade. * fix(deployments): clean up Heat stacks on FAILED-deployment delete (#168) * fix(deployments): clean up Heat stacks on FAILED-deployment delete Two bugs left OpenStack Heat stacks orphaned when a user deleted a failed deployment: 1. wait_for_status raising CREATE_FAILED never returned the stack id, so deploy_tasks could not record it in openstack_stack_id — the stack existed in OpenStack but the delete task had nothing to target. Now openstack_heat_service.create_stack stamps stack_id onto the exception, and deploy_tasks records it into created_stack_ids before marking the deployment FAILED. 2. delete_deployment removed the DB row even when Heat refused to delete the stack — orphaning the stack with no way to find it from the UI. Now if any stack delete fails, the DB row is kept, the surviving stack ids are persisted back, and the task returns status 'stack_delete_failed' so the user can retry. Adds tests/unit/test_delete_deployment_task.py covering the four cases: heat fails → row kept, heat ok → row gone, no stack id → row gone, partial failure → only failed stacks remain in openstack_stack_id. * fix(test): remove unused pytest import flagged by ruff F401 * feat(deployments): auto-resolve course groups on deployment create Move course_groups get-or-create out of the frontend wizard and into DeploymentService. For every group referenced by the incoming stack assignments we now look up an existing CourseGroup by (course_id, name) or create one, and backfill course_group_id on the request payload so deploy_tasks can stamp the FK onto DeploymentInstanceAccess rows. This guarantees students see their credentials via /api/v1/student/ even on the very first deployment of a course — previously the wizard had to bootstrap the course_groups rows itself, which could fail and either abort the deploy or leave group_id NULL on credential rows. * feat: publish-request flow + semver version validation Three coupled changes to fix the user-visible inconsistencies the team has been running into when adding templates and importing versions: ## 1) "Öffentlich" at creation no longer flips visibility immediately When a lecturer creates a template with visibility=public, the template now stays PRIVATE in the DB and carries a new `publish_requested=True` flag. The first version enters the standard approval flow (PENDING for lecturers, APPROVED for admin-callers). The atomic flip to visibility=PUBLIC happens inside approve_version() on the first admin approval; reject_version() clears the publish_requested wish so the owner has to re-initiate. Why a Boolean instead of a third visibility enum value: keeps the existing two-value enum and every access-control path (_can_access_template, _can_access_version) untouched. A simple boolean filter is enough for the admin approval queue. Migration `e3a91d7b5c42` adds the column AND backfills existing PUBLIC templates that have no APPROVED version yet — they were the bug; the data-correct state is PRIVATE+publish_requested. ## 2) Version strings must be semver, unique per template, strictly monotonic New `src/utils/version_validator.py` is the single source of truth: semver 2.0 regex, comparable tuple, and an `assert_strictly_greater()` helper that raises BadRequestException with structured codes the frontend branches on: - VERSION_NOT_SEMVER - VERSION_MISSING_IN_MANIFEST - VERSION_NOT_STRICTLY_GREATER - VERSION_ALREADY_EXISTS - VERSION_REPLACE_BLOCKED_BY_DEPLOYMENTS Validation now runs in github_import_service._import_version_for_template, template_version_service.create_version, .create_version_with_files, and .update_version — every path that persists a version row. `app.yaml` must carry `app.version` — the legacy `_derive_fallback_version` timestamp fallback is gone. When the manifest version collides with an existing row, the API returns VERSION_ALREADY_EXISTS with details so the UI can offer two branches: "bump in repo" (link) or "replace existing" (new `replace_existing=true` flag on the request). The replace path is blocked when active deployments still reference the row being replaced. Migration `ebc91d7b5d43` adds the UniqueConstraint on (template_id, version) plus a one-pass dedupe (`+dedupe-<sha>` suffix on duplicates' version strings) so the constraint can be applied on existing data. ## 3) Approval queue includes publish_requested templates `list_by_approval_status` now treats `visibility=public` as "PUBLIC OR (PRIVATE AND publish_requested)" by default so a freshly created publish-requested template's PENDING version doesn't fall out of the admin queue. Override via `include_publish_requested=false`. ## Structured BadRequestException `BadRequestException` now optionally carries `code` and `details`; the HTTP-exception-handler forwards both into the `errors` field of the standard envelope. The contract is additive: existing callers that pass only a message are unchanged. ## Tests - test_version_validator.py (31 cases): regex edge cases, ordering, error codes, replace-target semantics - test_template_publish_flow.py: initial_approval logic across the four (visibility, publish_requested) combinations + the approve/reject promote & demote-wish flips - test_template_version_activate.py: switch active version both up and down, no strict-newer gate - test_approval_only_for_public.py: updated to the new "private→public doesn't flip immediately" semantics - test_templates_api_access.py: fixture bumped to v1.1.0 so the new unique (template_id, version) constraint doesn't block it Whole suite: 462 passed, 4 skipped, 0 failures. * chore(tests): clean up unused imports + variables flagged by ruff CI ruff run flagged F401/F841 in the two new test files: - test_template_publish_flow.py: drop unused datetime/timezone + pytest imports (the fixtures + service methods stamp timestamps internally; these tests only assert behaviour, no pytest decorators needed) - test_template_version_activate.py: drop unused BadRequestException import; inline-construct the "other" version row without binding it to v_old/v_new so the comment makes the test's narrative obvious ("an older active version exists; we're switching to a newer one" without ruff complaining about the unused binding) * chore: fix mypy type errors in exceptions + version_validator CI mypy run flagged two type errors: - src/core/exceptions.py:128 — `errors_payload` was inferred as `dict[str, str]` from `{"code": exc.code}` (since `exc.code: str`), then re-assigning `errors_payload["details"] = exc.details` (a dict) broke. Annotate explicitly as `dict[str, Any] | None`. - src/utils/version_validator.py:70 — `parts` was inferred as `list[tuple[int, int]]` from the first append, then the str-branch failed. Annotate as `list[tuple[int, int | str]]` matching how the identifier kind drives the value type. No behaviour change. ruff still clean, mypy now passes on both files, test_version_validator / test_template_publish_flow / test_template_version_activate still green (44 tests). * fix(deployments): create User/CourseMember/GroupMember from wizard students (#172) The student self-service endpoint (GET /api/v1/student/deployments) inner- joins through users → course_members → group_members → course_groups → deployment_instance_access. Stamping group_id on the access row was necessary but not sufficient — no code path ever created the membership rows for the students named in the deployment wizard payload, so the inner joins returned empty and students saw nothing. DeploymentService.create_deployment now calls a new _sync_student_memberships helper after the CourseGroup backfill. The helper, per (course, group, student): - Looks up the User by external_id; creates one from the wizard's StudentInfo claims if missing (so a student who has never logged in is still wired up — UserSyncService will find and refresh the row on their next real login because both paths key on external_id). - Looks up the CourseMember by (user_id, course_id) with left_at IS NULL; creates one if missing. - Looks up the GroupMember by (group_id, course_member_id); creates one if missing. Idempotent — re-deploying with the same students adds nothing. Tests cover: new student creates all three rows; existing user is reused on external_id match (display fields preserved); second call is a no-op; a student in two groups gets one CourseMember and two GroupMembers. * fix(deployments): persist non-SSH app credentials per group and teacher Previously, deploy_tasks built credentials_for_db with only SSH/Linux credentials — the per-group loop filtered on s['linux']['password'], and the teacher block only emitted an SSH admin entry. For templates whose app.yaml declares per_group credentials of any other type (postgres, pgadmin, web_url, ...), the resulting user_json had an empty 'instance.credentials' list and no 'applications' section, so DeploymentInstanceAccess rows were only ever written for the teacher's auto-generated SSH key. Students saw nothing in the lecturer UI either (no Gruppen tab) because no group_id-stamped rows existed. Now deploy_tasks discovers every non-bookkeeping key in generated['deployment_groups'][*] and generated['teacher'] (i.e. everything except username/email/group_name/group_index/course_group_id/ students/linux — linux is still handled via the dedicated SSH section) and emits one applications[] entry per credential type. Each entry carries group_id pulled from course_group_id for groups, and explicit None for the teacher's admin_credentials. SSH handling is unchanged. In the credential service, _extract_access_entries now reads group_id from each application credential and sets it explicitly to None for admin_credentials. Without this, even a populated applications[] would have written rows with group_id NULL and remained invisible to students. Verified against the ansible-postgres-group-db template (per_group: postgres + pgadmin, teacher: postgres + pgadmin, no linux): two groups × two cred types now yields four group-stamped DATABASE rows and two admin DATABASE rows, plus the existing SSH admin row. * fix(student): surface group_id/group_name on student credential rows (#174) The student credentials endpoint returned all access entries with group_id=null and group_name=null even when the underlying rows had a group attribution. A student who is a legitimate member of multiple course groups on the same deployment (e.g. multi-group lab assignment) then saw 4 identical-looking rows with no way to tell which credential belonged to which group. The lecturer-facing endpoint already fills these fields (src/api/deployments.py:634-638). This mirrors that for the student endpoint. Same authorization scope as before — only rows where a.group_id is in the student's allowed_groups set are surfaced. * fix(deployments): garbage-collect orphan student users on deployment delete (#175) Nothing in the codebase ever deletes user / course_member / group_member rows — UserSyncService.deactivate_user is a comment-only no-op and isn't called from anywhere. Combined with _sync_student_memberships, which creates one User per student named in every wizard payload, those tables only grow. delete_deployment now does a final cleanup pass after the deployment row is removed: 1. Snapshot all student user_ids tied to the deployment BEFORE the access rows are deleted (we lose the chain afterwards). 2. _gc_orphan_student_memberships walks each user: - drops GroupMember rows whose course_group no longer has a live DeploymentInstanceAccess on any deployment - drops the CourseMember once its last GroupMember is gone - drops the User row once its last CourseMember is gone 3. Safety net: users that own templates or openstack projects are skipped — those are lecturers/admins, never students. This keeps the users/course_members/group_members tables bounded by the number of active deployments instead of growing monotonically. A student in two deployments stays until the second one is deleted; the cleanup re-queries the surviving access rows at each call so it's safe to chain. Tests cover: orphan student is dropped end-to-end; user with active access on another deployment is kept; template-owner / openstack-owner safety nets fire; empty input set is a no-op. * feat(deployments): allow admins to deploy private templates of other owners (#176) The private-template deploy gate previously refused everyone except the template owner — even admins. This was inconsistent with the rest of the service: admins bypass the owner check on get/update/delete and on every other admin-trust path, but not here. create_deployment now accepts is_admin (defaulted to False so existing callers stay correct) and the gate evaluates: private AND not-owner AND not-admin -> 403 The API endpoint reads UserRole.ADMIN from the token's roles claim (same pattern as the other endpoints) and passes it through. Public templates are unaffected — their visibility/approval system already controls access without an owner check. Tests cover: non-owner lecturer still blocked, owner still allowed, admin now allowed on a private template they don't own, public template unaffected. * feat(course-filters): admin-managed string filters for course names (#177) * feat(course-filters): admin-managed string filters for course names Adds a flat, admin-managed list of strings that the frontend renders as filter-chips above the course list. Filtering itself stays client-side — this resource only persists the labels. - Model + migration: course_filters table, name unique - Endpoints under /api/v1/course-filters: - GET list (paginated, ?search=) open to any authenticated user - POST / PATCH / DELETE — ADMIN only - Duplicate names mapped to 409 (pre-check + IntegrityError race fallback) - Tests: 15 cases covering happy paths, admin-only enforcement, duplicate handling, and 404s * fix(course-filters): tighten update/delete signatures to UUID mypy flagged str | UUID args being passed into BaseRepository.{update, delete}, which are annotated as UUID. The path parameters reaching these methods are already UUID (FastAPI parses them out of filter_id: UUID), so narrowing the service signatures matches the pattern in course_service and clears CI. * feat(course-filters): tighten PATCH/POST contract (required name, forbid extras) (#178) - POST and PATCH bodies now reject unknown keys (extra="forbid") so a typo or a stale frontend surfaces as 422 instead of being silently dropped (Pydantic's default). - PATCH.name is now REQUIRED, not Optional. The only editable field is name, and a body without it is a no-op — accepting {} hid client bugs that would only show up in prod. When a second editable field is added later, relax this back to Optional + add an at-least-one-set validator. - Service simplified: the unreachable empty-update branch is gone. - Tests: +4 cases (extra-fields rejected on POST/PATCH, empty PATCH body returns 422, same-name PATCH is a valid no-op that still hits update()). * feat(deployments): redeploy whole deployment or single VM with config overrides (#179) Adds two new endpoints that let lecturers re-apply a deployment from scratch — either across every VM in the class or targeted at one wedged group — and accept per-deployment / per-VM template parameter overrides so a config change actually takes effect (Heat + Ansible re-run, not just a Heat update_stack refresh). API POST /deployments/{id}/redeploy iterates sequentially over every DeploymentInstance row and destroy-and-recreates each VM. Body accepts: - deployment_parameter_overrides (apply to every VM) - instance_parameter_overrides (per-VM map, merged on top) - preserve_credentials (default false → fresh creds) POST /deployments/{id}/instances/{instance_id}/redeploy same flow for one VM only. Parent deployment stays RUNNING so sibling VMs remain reachable for students; only the targeted instance flips to the new REDEPLOYING status. Backend - Extracts _provision_one_stack_assignment as the shared 'create stack → wait SSH → run Ansible → persist credentials' helper. Both the initial deploy_stack loop and the new redeploy tasks call it, so drift between first-deploy and redeploy is impossible. - _merge_parameter_layers implements the override order base → deployment → instance (later layers win, backend-managed Heat params like user_json/key_name are always stripped). - _snapshot_instance_credentials / rebind path so preserve_credentials can keep students' logins working across a config change. - Adds DeploymentInstanceStatus.REDEPLOYING + Alembic migration e2a91d05c7b8 to extend the Postgres enum (IF NOT EXISTS, idempotent). Tests - 21 unit tests for the merge logic, vm_name → stack_assignment reconstruction, credential snapshot/rebind, orchestration, and redeploy_deployment fan-out (incl. partial-failure path). - 9 API tests for the two endpoints (success, defaults, 400 in transitional state, 404 paths). Bruno - bruno/Deployments/Redeploy Deployment.bru - bruno/Deployments/Redeploy Instance.bru Both with full docs covering body schema, merge order, error cases, and the preserve_credentials caveat. * fix(deployments): close orphan-stack window during (re)deploy (#180) Closes a race where a worker crash between Heat's create_stack and the final commit that flushes the new stack id onto deployment.openstack_stack_id would leave the Heat stack alive in OpenStack with no DB pointer for delete_deployment to find. Affected both deploy_stack (between stack N and the end-of-loop final commit) and redeploy_instance (the entire multi-minute Ansible phase sat inside this window — the bigger of the two). Two safeguards, layered: 1. _provision_one_stack_assignment gets a new on_stack_created callback, invoked IMMEDIATELY after heat_service.create_stack returns and BEFORE credential persistence + Ansible. Both call sites pass a closure that commits the new stack id onto deployment.openstack_stack_id right then. Failures inside the callback are logged and swallowed — the later final commit is still the source of truth. 2. delete_deployment now collects stack ids from BOTH deployment.openstack_stack_id AND DeploymentInstance.openstack_server_id via the new _collect_stack_ids_for_cleanup helper. Even if safeguard #1 ever fails (or an orphan from a previous build of the code survives), the cleanup still finds the stack through the instance row, which is committed inside the same transaction that creates the row. Tests - 9 new unit tests in tests/unit/test_orphan_stack_safeguards.py covering the callback ordering (must run before credential persist), failure-isolation, and every shape _collect_stack_ids_for_cleanup handles (null / empty-array / legacy bare id / union with instance rows / errored instance query). - Existing test_delete_deployment_task.py + test_redeploy_tasks.py still green, total 529 passed. * Feat/lecturer management endpoints (#181) * feat(admin): /lecturers list + detail + async cascade delete Adds admin-only endpoints for lecturer account management. No API user role is stored in our DB (Keycloak is source-of-truth), so 'lecturer' is defined structurally as 'a user who owns templates or OpenStack projects.' Students never satisfy this — they cannot create either. Endpoints (all guarded by require_roles(UserRole.ADMIN)): * GET /api/v1/lecturers paginated list with template / deployment / OSP counts * GET /api/v1/lecturers/{user_id} detail view with the full owned/ deployed resource lists * DELETE /api/v1/lecturers/{user_id} enqueues cascade_delete_lecturer Celery task, returns 202 with the task id and the summary counts Cascade ordering (in the Celery task): 1. delete_deployment.apply(...) for each deployment. If ANY reports stack_delete_failed or raises, the cascade aborts — the user + all their resources survive so the admin can inspect and retry. Same bail-out contract as single-deployment delete. 2. TemplateService.delete_template(is_admin=True) for each template — reuses the existing versions/files/approvals cascade. 3. Drop the OpenstackProject DB rows (Keystone itself is untouched). 4. Drop the User row. Deployment ownership lives in deployment_parameters JSON rather than a FK column. LecturerService selects via a JSONB path expression on Postgres and falls back to per-row Python parsing on SQLite, so unit tests exercise the same ownership logic without needing a real Postgres. Self-delete is refused up-front (BadRequestException). Cascade aborts never remove the user row, so retrying after fixing the failing Heat stack is always safe. * chore(lecturers): fix ruff F401 and mypy var-annotated on lecturer service - Remove unused UUID import from lecturer_tasks.py (F401). - Annotate version_counts as dict[str, int] and build via comprehension so mypy accepts the Row -> dict conversion. * Feat/template icon upload (#182) * feat(templates): file upload for template icons Neue Funktion, mit der Owner/Admins ein Icon-Bild für ein Template hochladen können. Bislang lief ``icon_url`` als reiner String — externe URL, ``mdi:*``/``fa:*``-Identifier oder Emoji. Für hochgeladene Assets gab es keinen Weg. Design: - Neue Tabelle ``template_icons`` mit BYTEA-Content (Bild-Bytes), ``content_type``, ``file_name``, ``size_bytes``. 1:1 zu Templates via unique FK + ON DELETE CASCADE. - ``content``-Spalte ist SQLAlchemy-``deferred`` — Blob wird nur beim Serve-Endpoint aus der DB gezogen, nie bei normalen Template-Queries. - Response bekommt ``effective_icon`` (computed): hochgeladenes Icon → ``/api/v1/templates/{id}/icon``, sonst Fallback auf ``icon_url``, sonst ``None``. Rohfeld ``icon_url`` bleibt sichtbar für Edit-UIs. ``has_uploaded_icon`` als billiges Signal fürs Frontend. - Drei neue Endpoints: POST /templates/{id}/icon (multipart, owner-or-admin) GET /templates/{id}/icon (Serve, Sichtbarkeits-Gate wie GET Template) DELETE /templates/{id}/icon (owner-or-admin, idempotent) - Validierung im Service: PNG/JPEG/WebP whitelist (415 sonst), max 5 MB (413 sonst), leere Uploads → 400. Grenzwerte via ``settings.max_icon_size_bytes`` / ``allowed_icon_content_types``. - ``POST /templates`` und ``PATCH /templates`` bleiben pure-JSON — keine Breaking Changes an bestehenden Aufrufen. Migration ``c8a3f1e9b7d5`` legt die neue Tabelle an. ``icon_url`` auf ``templates`` bleibt unverändert. Tests: 23 Unit-Tests (Service + Schema), 14 API-Tests (Routes inkl. Cascade-Delete). Ruff/Mypy grün. * refactor(templates): drop icon_url — upload-only icons Icons kommen ab jetzt ausschließlich als hochgeladenes Bild. Das alte ``icon_url``-String-Feld (``mdi:*``, externe URLs, Emoji) wird komplett entfernt — an keinem Endpoint mehr entgegengenommen, nicht mehr in der Response, aus der DB gedroppt. Grund: die einzige Quelle, die ``icon_url`` je gesetzt hat, waren die zwei Seed-Templates. Die ``app.yaml``-Dateien in DoziLab/appstore-apps haben kein Icon-Feld, der AppManifestParser liest auch keins, und Nutzer haben es beim Anlegen praktisch nie manuell gepflegt. Konsistenter ist „entweder ein hochgeladenes Bild oder Placeholder". Änderungen: - Migration ``c8a3f1e9b7d5`` erweitert um ``op.drop_column('templates', 'icon_url')`` — Feature-Branch ist noch nicht deployed, also kein Bestand zu retten, eine atomare Migration. - Model, Schemas (Create/Update/Response/GithubImport) und Services entfernen das Feld. ``TemplateResponse.effective_icon`` fällt nicht mehr auf ``icon_url`` zurück — ohne Upload ist der Wert ``null``. - Seed-Daten für „Multi-User Ubuntu" und „PostgreSQL Group DB" verlieren ihre ``mdi:*``-Werte; nach diesem Change zeigen die Kacheln erstmal einen Placeholder, bis jemand ein Bild hochlädt. - Bruno-Requests (Create Template, Import Template From GitHub) senden das Feld nicht mehr mit; Docs aktualisiert. - Tests: 3 Test-Files angepasst, ``test_template_effective_icon_schema`` auf die zwei relevanten Zweige (Upload / kein Upload) reduziert. Tests: 573 passed, 4 skipped. Ruff + mypy grün. * refactor(templates): rename effective_icon → icon_path, drop has_uploaded_icon Nach dem Wegfall der icon_url-Alternative gibt es nur noch eine Quelle für ein Template-Icon: den Upload. Der Name ``effective_icon`` machte nur Sinn, solange das Feld zwischen zwei Kandidaten (Upload vs. mdi/URL-String) wählte — jetzt ist es einfach der Pfad zum Icon-Bild oder null. Zusätzlich: streng genommen ist der Wert kein URL, sondern ein relativer Pfad (kein Scheme, kein Host). Deshalb ``icon_path`` statt ``icon_url`` — der Client muss ihn ohnehin gegen seine API-Base-URL auflösen. ``has_uploaded_icon`` fällt weg — das Flag trägt exakt dieselbe Info wie ``icon_path !== null`` und ist reines Rauschen im Response-Body. Response-Feld im POST-Upload-Endpoint ebenfalls von ``url`` auf ``icon_path`` umbenannt für Konsistenz. Tests: 573 passed. Ruff + mypy grün. --------- Co-authored-by: Dilmand Zoro <dilmand@outlook.de> Co-authored-by: Dilmand Zoro <84354503+Dilmand@users.noreply.github.com> Co-authored-by: Dilmand <dilmand@users.noreply.github.com> Co-authored-by: Sergio Meli <sergio01@hotmail.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.