diff --git a/README.md b/README.md index 40cde1a..3eafaf8 100644 --- a/README.md +++ b/README.md @@ -215,6 +215,39 @@ This should start multiple `gunicorn` workers, each one of them binding our flas > Reminder: Replace the **``** in the commands above either with `preprint` or `preview` depending on the server (e.g., `neurolibre-preview.service`) you are configuring. Note that this is not only a naming convention, but also defines a functional separation between the roles of the two servers. +#### Isolate MyST build containers from instance metadata + +Build containers execute notebook code from submitted repositories. On the default Docker bridge they can reach the instance metadata service (`169.254.169.254` on OpenStack), which serves user-data and injected credentials. + +Create a dedicated network with a fixed bridge name, so the firewall rule has something stable to match: + +``` +docker network create --driver bridge \ + --opt com.docker.network.bridge.name=br-mystbuild mystbuild +docker pull busybox:latest +``` + +Install the service that blocks metadata for that bridge on every boot: + +``` +sudo cp ~/full-stack-server/systemd/neurolibre-mystbuild-firewall.service /etc/systemd/system/ +sudo systemctl daemon-reload +sudo systemctl enable --now neurolibre-mystbuild-firewall.service +``` + +Verify — this is the only thing that proves the rule is working: + +``` +docker run --rm --network mystbuild curlimages/curl \ + -s -m 3 http://169.254.169.254/openstack/ ; echo "exit=$?" +``` + +A non-zero exit (`7` rejected, `28` timeout) means blocked. `exit=0` with a listing of API versions means it is **not** blocked — check that `br-mystbuild` exists (`ip -o link show br-mystbuild`) and that metadata is not a local address (`ip addr | grep 169.254`, which should print nothing). + +Then pass `container_network = 'mystbuild'` to `JupyterHubLocalSpawner` in `api/neurolibre_celery_tasks.py` and restart the Celery worker. myst-libre re-checks this before every build session and refuses to spawn if metadata answers, so a rule lost after a reboot fails loudly instead of silently reopening. + +> Do not use `iptables-persistent` for this rule. It snapshots the entire ruleset including Docker's generated rules, and restoring those at boot before Docker starts causes duplicated and conflicting rules. The systemd unit is ordered after `docker.service` and re-adds only this rule. + #### Configure Celery as a systemd service For Celery async task queue manager to work, there are two requirements: diff --git a/about.md b/about.md index 65db558..23eb03a 100644 --- a/about.md +++ b/about.md @@ -87,6 +87,7 @@ celery -A neurolibre_celery_tasks worker --loglevel=info The application runs as systemd services: - `neurolibre-preview.service` - Preview server - `neurolibre-preprint.service` - Preprint server +- `neurolibre-mystbuild-firewall.service` - Blocks instance metadata for build containers - Celery workers for async tasks ## Key Directories diff --git a/api/common.py b/api/common.py index 6a9b36a..2b78ac4 100644 --- a/api/common.py +++ b/api/common.py @@ -541,13 +541,6 @@ def run_celery_subprocess(command, log_output=True): logging.error(f"Command: {' '.join(command)}") return -1, str(e) -def get_active_ports(start=3001, end=3099): - active_ports = [] - for conn in psutil.net_connections(kind='inet'): - if conn.status == psutil.CONN_LISTEN and start <= conn.laddr.port <= end: - active_ports.append(conn.laddr.port) - return active_ports - def close_port_by_pid(target_pid): """Kill the entire process group rooted at target_pid. diff --git a/api/github_client.py b/api/github_client.py index 846d360..9f04839 100644 --- a/api/github_client.py +++ b/api/github_client.py @@ -1,9 +1,11 @@ import os import re -from common import get_time +import logging +from common import get_time, parse_front_matter import json import yaml import git +from myst_frontmatter import merge_paper_metadata # Name of the GitHub organization where repositories # will be forked into for production. Editorial bot @@ -330,6 +332,29 @@ def gh_get_paper_markdown(github_client,repo): file_content = gh_get_file_content(github_client,repo,"paper.md") return file_content +def gh_get_paper_metadata(github_client, repo): + """Paper metadata for a submission, with myst.yml filling any gaps. + + NeuroLibre requires myst.yml at the repository root beside paper.md, so a + submission need not repeat its title, authors, and affiliations in the + paper.md front matter. Returns None only when neither source names an + author. + + This runs before the repository is cloned, so both files are fetched + through the GitHub API rather than read from disk. + """ + paper = gh_get_file_content(github_client, repo, "paper.md") + + front_matter = None + if paper: + try: + front_matter = parse_front_matter(paper) + except yaml.YAMLError as error: + logging.warning(f"Could not parse paper.md front matter: {error}") + + myst = gh_get_file_content(github_client, repo, "myst.yml") + return merge_paper_metadata(front_matter, myst) + def gh_read_from_issue_body(github_client,issue_repo,issue_id,tag): """ Issue body of the reviews has markers around review entries diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py new file mode 100644 index 0000000..4eff044 --- /dev/null +++ b/api/myst_frontmatter.py @@ -0,0 +1,296 @@ +"""Translate a myst.yml `project` mapping into inara paper metadata. + +A NeuroLibre submission declares its title, authors, and affiliations in +myst.yml for the living preprint. The publishing pipeline wants the same +information in the shape the paper.md front matter uses: affiliations numbered +by index, and each author's affiliations as a comma-joined string of those +indices. + +This module is pure. Fetching files is the caller's job, which keeps the +mapping testable without a GitHub client. It mirrors +inara/data/filters/myst-frontmatter.lua; the two share the mapping documented +in the design spec. It also merges a parsed paper.md front matter with a +myst.yml, filling any gaps the front matter leaves. +""" + +import logging + +import yaml + +# Parts of a myst.yml affiliation, joined into one name string. Department +# precedes institution to match the convention in existing NeuroLibre front +# matter. +NAME_PARTS = ( + "department", + "institution", + "address", + "city", + "region", + "postal_code", + "country", +) + +# MyST accepts these aliases for two of the parts. +ALIASES = {"institution": "name", "region": "state"} + + +def _is_blank(value): + """Is a value absent, or present but carrying nothing? + + A `paper.md` front matter of `title:` parses to `title: None`, not to a + missing key. `""`, `[]` and `{}` say the same thing. All of them must count + as absent or a key that was merely typed out defeats the myst.yml fallback. + Mirrors `is_blank` in inara's myst-frontmatter.lua. + """ + if value is None: + return True + if isinstance(value, str): + return not value.strip() + if isinstance(value, (list, tuple, dict, set)): + return len(value) == 0 + return False + + +def _as_list(value): + """Normalise a myst.yml sequence to a list. + + `affiliations: harvard` is legal MyST. Without this, iterating the string + would walk its characters. Mirrors `as_list` in myst-frontmatter.lua. + """ + if value is None: + return [] + if isinstance(value, (list, tuple)): + return list(value) + return [value] + + +def _affiliation_name(affiliation): + """Join an affiliation's parts into a single display string.""" + parts = [] + for key in NAME_PARTS: + value = affiliation.get(key) + if value in (None, "") and key in ALIASES: + value = affiliation.get(ALIASES[key]) + if value not in (None, ""): + parts.append(str(value).strip()) + return ", ".join(parts) + + +def _affiliation_tokens(value): + """Normalise an author's `affiliations` value to a list of tokens. + + MyST accepts a list, a single id, or several ids in one ';'-separated + string. + """ + if value in (None, ""): + return [] + if isinstance(value, (list, tuple)): + return [str(entry).strip() for entry in value if str(entry).strip()] + return [token.strip() for token in str(value).split(";") if token.strip()] + + +def myst_project_metadata(project): + """Return inara paper metadata derived from a myst.yml `project` mapping. + + Returns only the keys the project actually defines, so the caller can treat + the result as a set of defaults to fill gaps with. Junk input yields an + empty dict: this fallback must never be why a deposit fails. + """ + if not isinstance(project, dict): + return {} + + metadata = {} + + if not _is_blank(project.get("title")): + metadata["title"] = project["title"] + if not _is_blank(project.get("date")): + # `date: 2024-01-15` -- unquoted ISO, the MyST-canonical form -- is + # parsed by yaml.safe_load into a datetime.date. This value is carried + # into a Celery task payload, which is serialized as JSON, so a date + # object here is an HTTP 500 at enqueue time. Nothing downstream + # consumes `date` structurally, so the string form is the right shape. + date = project["date"] + metadata["date"] = date if isinstance(date, str) else str(date) + if not _is_blank(project.get("keywords")): + metadata["tags"] = project["keywords"] + if not _is_blank(project.get("bibliography")): + metadata["bibliography"] = project["bibliography"] + + affiliations = [] + index_of = {} + for source in _as_list(project.get("affiliations")): + index = len(affiliations) + 1 + if not isinstance(source, dict): + # MyST's validator accepts a bare string where an affiliation + # mapping is expected. It becomes an affiliation named after that + # string, with no id, and it still consumes its index position -- + # the Lua filter applies the same rule, so both sides agree on + # every author's index. + affiliations.append({"index": index, "name": str(source).strip()}) + continue + affiliations.append({"index": index, "name": _affiliation_name(source)}) + if source.get("id") is not None: + index_of[str(source["id"])] = index + + authors = [] + for source in _as_list(project.get("authors")): + if not isinstance(source, dict): + # Same MyST rule for authors: `authors: [Ada Lovelace]` is valid. + # A bare string becomes a named author with no affiliations, still + # holding its position in the list. + authors.append({"name": str(source).strip()}) + continue + author = {"name": source.get("name")} + for target, key in ( + ("email", "email"), + ("orcid", "orcid"), + ("corresponding", "corresponding"), + ("equal-contrib", "equal_contributor"), + ): + if source.get(key) is not None: + author[target] = source[key] + + indices = [] + tokens = _affiliation_tokens( + source.get("affiliations") or source.get("affiliation") + ) + for token in tokens: + index = index_of.get(token) + if index is None: + # MyST permits ad-hoc affiliations. Inventing an entry beats + # dropping the author's affiliation. + index = len(affiliations) + 1 + affiliations.append({"index": index, "name": token}) + index_of[token] = index + indices.append(str(index)) + if indices: + author["affiliation"] = ",".join(indices) + + authors.append(author) + + if authors: + metadata["authors"] = authors + if affiliations: + metadata["affiliations"] = affiliations + + return metadata + + +def merge_paper_metadata(front_matter, myst_text): + """Paper metadata from paper.md, with myst.yml filling any gaps. + + `front_matter` is the already-parsed paper.md front matter, or None for a + paper that has none. `myst_text` is the raw contents of myst.yml, or None. + Parsing myst.yml happens here rather than in the caller so that a malformed + file is tolerated in one place. + + Returns None when neither source names any author — the same signal the + deposit path already treats as "cannot extract metadata". + """ + metadata = dict(front_matter) if isinstance(front_matter, dict) else {} + + if myst_text: + try: + project = (yaml.safe_load(myst_text) or {}).get("project") + except yaml.YAMLError as error: + logging.warning(f"Could not parse myst.yml: {error}") + project = None + except AttributeError: + # yaml.safe_load returned something that is not a mapping. + project = None + fallback = myst_project_metadata(project) + + # Authors and affiliations are filled as a pair. An affiliation index + # only means something relative to the list that defines it, so mixing + # the two sources would silently attach authors to the wrong + # institutions. + # + # A key that is present but empty counts as absent -- see `_is_blank`. + if _is_blank(metadata.get("authors")) or _is_blank(metadata.get("affiliations")): + if fallback.get("authors"): + if not _is_blank(metadata.get("authors")): + # The front matter named authors but no affiliations, so its + # author list is discarded rather than merged. Announce it: + # a stale myst.yml silently outranking a current paper.md is + # otherwise indistinguishable from a correct fallback. + logging.warning( + "paper.md names authors but no affiliations; replacing " + "its author list with the one from myst.yml, because an " + "affiliation index only means something relative to the " + "list that defines it." + ) + metadata["authors"] = fallback["authors"] + metadata["affiliations"] = fallback.get("affiliations", []) + for key in ("title", "date", "tags", "bibliography"): + if _is_blank(metadata.get(key)) and key in fallback: + metadata[key] = fallback[key] + + if not metadata.get("authors"): + return None + return metadata + + +def first_affiliations(authors, affiliations): + """Resolve each author's first affiliation to a display name. + + `authors` is a list of author dicts as produced by `merge_paper_metadata` + (or a hand-written paper.md front matter); each may carry an `affiliation` + value that is an int, a comma-separated string of indices, an empty + string, or absent entirely. `affiliations` is the corresponding list of + `{"index": ..., "name": ...}` mappings. + + Returns a list the same length as `authors`. An element is `None` when the + author has no affiliation, or names an index the affiliation list does not + define -- both are legitimate, not errors: myst.yml permits an author with + no affiliation (see `test_author_without_affiliations_gets_no_affiliation_key`), + and a caller should not have the deposit fail just because one author + lacks one. + + An empty `affiliations` list is legitimate too -- a myst.yml project may name + authors and no institutions at all -- and resolves every author to `None`. + """ + # Built with `.get`, not subscripting: a hand-written paper.md may omit + # `index` or `name` on one entry, and that entry alone should be unusable + # rather than raising and failing the deposit. + mapping = {} + for affiliation in affiliations or []: + if not isinstance(affiliation, dict): + continue + index = affiliation.get("index") + name = affiliation.get("name") + if index is None or name is None: + logging.warning( + f"Ignoring an affiliation entry missing 'index' or 'name': " + f"{affiliation!r}." + ) + continue + mapping[str(index).strip()] = name + + resolved = [] + for author in authors: + if not isinstance(author, dict): + # `authors: [Ada Lovelace]` is legal in both sources; a bare string + # names no affiliation. + resolved.append(None) + continue + affiliation = author.get("affiliation") + if not affiliation: + resolved.append(None) + continue + if isinstance(affiliation, int): + affiliation_index = affiliation + else: + # `affiliation: "1, 2"` is as common as `"1,2"` in front matter. + affiliation_index = str(affiliation).split(",")[0].strip() + name = mapping.get(str(affiliation_index).strip()) + if name is None: + # A typo'd index used to crash loudly; now it silently records a + # creator with no institution. Say so, so it is diagnosable. + logging.warning( + f"Affiliation index {affiliation_index!r} for author " + f"{author.get('name')!r} is not defined by the affiliation " + f"list; recording no affiliation for this author." + ) + resolved.append(name) + + return resolved diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 598f704..e23d60d 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -1,4 +1,5 @@ from celery import Celery +from celery.signals import celeryd_after_setup import time import os import json @@ -7,6 +8,7 @@ from celery import states from github_client import * from screening_client import ScreeningClient +from myst_frontmatter import first_affiliations from common import * from preprint import * from github import Github, UnknownObjectException, GithubException @@ -21,7 +23,9 @@ from repo2data.repo2data import Repo2Data from myst_libre.tools import JupyterHubLocalSpawner from myst_libre.rees import REES +from myst_libre.exceptions import MystLibreError from myst_libre.builders import MystBuilder +from myst_libre.tools import MystMD from celery.schedules import crontab import zipfile import tempfile @@ -119,6 +123,34 @@ # DB 0 is the Celery broker; we use DB 2 for locks to avoid key collisions. _lock_redis = redis_lib.Redis(host='localhost', port=6379, db=2) + +@celeryd_after_setup.connect +def reap_myst_orphans(sender, instance, **kwargs): + """ + Clean up myst process groups left behind by a previous worker. + + A crashed or restarted worker leaves myst and its children (npm run start -> + node ./server.js) running and holding ports. Normal teardown kills the + process group, but that needs a live PID to signal. + + celeryd_after_setup fires once in the main worker process, after setup and + before children fork or any task is consumed. worker_process_init would be + wrong here: it runs in every prefork child, so N reaps would race. + + Builds running in sibling workers are unaffected - myst-libre only reaps + records whose owning process is gone. + """ + try: + reaped = MystMD.reap_orphans() + if reaped: + logging.warning( + f"Reaped {len(reaped)} orphaned myst process group(s): " + f"{[e.get('build_dir') for e in reaped]}" + ) + except Exception as e: + # Never block worker startup over cleanup + logging.warning(f"Orphan reaping failed: {e}") + """ Configuration END """ @@ -989,47 +1021,24 @@ def zenodo_create_buckets_task(self, payload): data = payload['paper_data'] - # We need to go through some affiliation mapping here. - affiliation_mapping = {str(affiliation['index']): affiliation['name'] for affiliation in data['affiliations']} - first_affiliations = [] - for author in data['authors']: - if isinstance(author['affiliation'],int): - affiliation_index = author['affiliation'] - else: - affiliation_indices = [affiliation_index for affiliation_index in author['affiliation'].split(',')] - affiliation_index = affiliation_indices[0] - first_affiliation = affiliation_mapping[str(affiliation_index)] - first_affiliations.append(first_affiliation) + # We need to go through some affiliation mapping here. The affiliation list + # can be absent entirely -- authors in the front matter plus a myst.yml + # project that names none -- so do not index it directly. + resolved_affiliations = first_affiliations(data['authors'], data.get('affiliations') or []) for ii in range(len(data['authors'])): - data['authors'][ii]['affiliation'] = first_affiliations[ii] - - # To deal with some typos, also with orchid :) - valid_field_names = {'name', 'orcid', 'affiliation'} - for author in data['authors']: - invalid_fields = [] - for field in author: - if field not in valid_field_names: - invalid_fields.append(field) - - for invalid_field in invalid_fields: - valid_field = None - for valid_name in valid_field_names: - if valid_name.lower() in invalid_field.lower() or (valid_name == 'orcid' and invalid_field.lower() == 'orchid'): - valid_field = valid_name - break - - if valid_field: - author[valid_field] = author.pop(invalid_field) - - if 'equal-contrib' in author: - author.pop('equal-contrib') - - if 'corresponding' in author: - author.pop('corresponding') + # A bare string author (`authors: [Ada Lovelace]`) is legal in both + # sources and carries no affiliation to resolve. + if not isinstance(data['authors'][ii], dict): + continue + if resolved_affiliations[ii] is None: + data['authors'][ii].pop('affiliation', None) + else: + data['authors'][ii]['affiliation'] = resolved_affiliations[ii] - # if author.get('orcid') is None: - # author.pop('orcid') + # Author fields are not filtered here: `zenodo_create_bucket` reduces them + # to the fields a Zenodo creator accepts (see `zenodo_metadata`), so the + # deposit boundary owns that rule and every caller gets it. collect = {} for archive_type in payload['archive_assets']: @@ -1254,20 +1263,44 @@ def zenodo_upload_docker_task(self, screening_dict): task.fail(f"ERROR: Unrecognized archive type.") else: - # try: - rees_resources = REES(dict( - registry_url=BINDER_REGISTRY, - gh_user_repo_name = f"{GH_ORGANIZATION}/{task.repo_name}", - gh_repo_commit_hash = commit_fork, - binder_image_tag = commit_fork, - binder_image_name = None, - dotenv = task.get_dotenv_path())) - - if rees_resources.search_img_by_repo_name(): + # REES discovers the image in its constructor, and since myst-libre + # 0.4.1 a missing one raises ImageNotFoundError instead of reporting + # False. Unhandled, that escaped as a bare traceback: Celery marked the + # task failed but nothing told GitHub, so the issue comment sat orange + # forever with no indication anything had gone wrong. + try: + rees_resources = REES(dict( + registry_url=BINDER_REGISTRY, + gh_user_repo_name = f"{GH_ORGANIZATION}/{task.repo_name}", + # The registry host doubles as the repository namespace -- the + # "registry url entered twice" noted below -- so the image lives + # at registry.evidencepub.io/binder--, not at + # binder--. bh_project_name is what prepends it. + # Without it the tags/list lookup 404s on an image that exists, + # which preview_build_myst_task gets right and this did not. + bh_project_name = BINDER_REGISTRY.split('https://')[-1], + gh_repo_commit_hash = commit_fork, + binder_image_tag = commit_fork, + binder_image_name = None, + dotenv = task.get_dotenv_path())) + + # No second lookup: the constructor above already discovered the + # image and raises when it is absent, so reaching here means it was + # found. search_img_by_repo_name lives on the registry client, not + # on REES, and calling it here raised AttributeError. logging.info(f"🐳 FOUND IMAGE... ⬇️ PULLING {rees_resources.found_image_name}") rees_resources.pull_image() - else: - task.fail(f"Failes REES docker image pull for {fork_url}") + except MystLibreError as exception: + task.fail(f"Cannot pull the docker image for {fork_url} from {BINDER_REGISTRY}: {exception}") + return + except Exception as exception: + # This task's only channel to the submitter is the issue comment. + # Anything unhandled here used to leave it orange forever while + # Celery logged a traceback nobody was watching, so report the + # class of the error too rather than letting it escape. + task.fail(f"Unexpected error preparing the docker image for {fork_url}: " + f"{exception.__class__.__name__}: {exception}") + return # except: @@ -2038,16 +2071,28 @@ def preview_build_myst_task(self, screening_dict): # Always clean up the myst process tree (kills the entire process # group: myst node + npm run start + node ./server.js) and the # JupyterHub container, regardless of success or failure. + # + # Each step is guarded independently. Previously an exception in the + # first one aborted the rest of this block, leaking the container AND + # the build lock - which then blocks every build of that repo until the + # 6000s timeout expires. A failed cleanup step must not cost more than + # itself. if builder is not None: - builder.cleanup() - cleanup_hub(hub) + try: + builder.cleanup() + except Exception as e: + logging.warning(f"builder.cleanup() failed: {e}") + try: + cleanup_hub(hub) + except Exception as e: + logging.warning(f"cleanup_hub() failed: {e}") try: build_lock.release() except redis_lib.exceptions.LockNotOwnedError: # Lock expired (build exceeded timeout) and was auto-released. logging.warning(f"Build lock {lock_key} already expired.") - except Exception: - pass + except Exception as e: + logging.warning(f"Could not release build lock {lock_key}: {e}") @celery_app.task(bind=True) @handle_soft_timeout diff --git a/api/neurolibre_preprint_api.py b/api/neurolibre_preprint_api.py index db5149c..5823f5f 100644 --- a/api/neurolibre_preprint_api.py +++ b/api/neurolibre_preprint_api.py @@ -328,16 +328,26 @@ def api_zenodo_post(user,id,repository_url): # We need the list of authors and their ORCID, this will # be fetched from the paper.md in the tarhet repository - paper_string = gh_get_paper_markdown(github_client,repository_url) - paper_data = parse_front_matter(paper_string) + paper_data = gh_get_paper_metadata(github_client,repository_url) if not paper_data: - comment = f"🔴 Cannot extract metadata from the front-matter of the `paper.md` for {repository_url}." + comment = f"🔴 Cannot extract metadata from the `paper.md` front-matter or the `myst.yml` for {repository_url}." gh_create_comment(github_client,REVIEW_REPOSITORY,issue_id,comment) return make_response(jsonify(f"Problem with parsing paper.md for {repository_url}"),404) + # `paper_data` is guaranteed to name authors and nothing else. The deposit + # needs a title too, so check it here: missing it deeper in the task means a + # KeyError inside Celery, which the author never sees. + if not paper_data.get('title'): + comment = f"🔴 Cannot determine the title of the submission from the `paper.md` front-matter or the `myst.yml` for {repository_url}." + gh_create_comment(github_client,REVIEW_REPOSITORY,issue_id,comment) + return make_response(jsonify(f"Missing title for {repository_url}"),404) + task_title = "Reproducibility Assets - Create Zenodo buckets" - comment_id = gh_template_respond(github_client,"pending",task_title,REVIEW_REPOSITORY,issue_id,paper_data['authors']) + # No task id yet -- one is stamped onto this comment by the "received" phase + # below. It used to be passed the author list, which rendered it as the task + # id in the pending comment. + comment_id = gh_template_respond(github_client,"pending",task_title,REVIEW_REPOSITORY,issue_id) celery_payload = dict(task_title = task_title, issue_id= issue_id, diff --git a/api/preprint.py b/api/preprint.py index e5c61da..a1b2822 100644 --- a/api/preprint.py +++ b/api/preprint.py @@ -7,6 +7,7 @@ import re from github import Github from github_client import gh_read_from_issue_body +from zenodo_metadata import zenodo_creators import csv import subprocess import nbformat @@ -76,7 +77,9 @@ def zenodo_create_bucket(title, archive_type, creators, repository_url, issue_id data = {} data["metadata"] = {} data["metadata"]["title"] = f"({tmp_type}) {title}" - data["metadata"]["creators"] = creators + # Whatever the submission declared, a creator is only what Zenodo accepts. + # Enforced here rather than in the caller so every deposit path is covered. + data["metadata"]["creators"] = zenodo_creators(creators) data["metadata"]["keywords"] = ["canadian-open-neuroscience-platform","neurolibre"] # (A) NeuroLibre artifact is a part of (isPartOf) the NeuroLibre preprint (B 10.55458/NeuroLibre.issue_id) data["metadata"]["related_identifiers"] = [{"relation": "isPartOf","identifier": f"{DOI_PREFIX}/{DOI_SUFFIX}.{issue_id:05d}","resource_type": "publication-preprint"}] diff --git a/api/zenodo_metadata.py b/api/zenodo_metadata.py new file mode 100644 index 0000000..8c9a1e1 --- /dev/null +++ b/api/zenodo_metadata.py @@ -0,0 +1,115 @@ +"""Reduce paper metadata to the shape a Zenodo deposit accepts. + +Paper metadata reaches the deposit path from two sources -- the paper.md front +matter and, filling its gaps, the myst.yml project (see `myst_frontmatter`). +Both describe authors more richly than Zenodo's legacy deposit schema allows a +creator to be: myst.yml authors routinely carry `email`, `github`, `twitter`, +`url` and `corresponding`, none of which Zenodo's `creators` accepts. Sending +them risks a validation error on the deposit, and an email address in +particular would be published on a public record. + +So the deposit boundary decides what a creator is, rather than trusting +whatever the submission happened to declare. This module is pure; the caller +fetches and merges. +""" + +import logging + +# The legacy Zenodo deposit schema for one entry of `metadata.creators`. +# Ordered, because a misspelled key is repaired by scanning this sequence and +# the first match wins -- iterating a set here made the repair depend on hash +# order, so the same author could map differently between runs. +ZENODO_CREATOR_FIELDS = ("name", "affiliation", "orcid", "gnd") + +# Misspellings seen in submissions that substring matching cannot repair. +CREATOR_FIELD_TYPOS = {"orchid": "orcid"} + + +def _is_blank(value): + """Is a value absent, or present but carrying nothing? + + Mirrors `myst_frontmatter._is_blank`: a key that was typed out but left + empty must not reach Zenodo as an empty creator field. + """ + if value is None: + return True + if isinstance(value, str): + return not value.strip() + if isinstance(value, (list, tuple, dict, set)): + return len(value) == 0 + return False + + +def _canonical_creator_field(key): + """Which Zenodo creator field does an author key mean, if any? + + Returns None for a key with no Zenodo counterpart -- `email` and friends -- + which is how they get dropped. + """ + lowered = str(key).strip().lower() + if lowered in ZENODO_CREATOR_FIELDS: + return lowered + if lowered in CREATOR_FIELD_TYPOS: + return CREATOR_FIELD_TYPOS[lowered] + for field in ZENODO_CREATOR_FIELDS: + # Catches `affiliations`, `author name`, and similar near misses. + if field in lowered: + return field + return None + + +def zenodo_creators(authors): + """Return `authors` as Zenodo creators, carrying only accepted fields. + + `authors` is a list of author mappings as produced by `merge_paper_metadata` + (or written by hand in a paper.md front matter), with affiliations already + resolved to display names by `first_affiliations`. A bare string is accepted + where a mapping is expected, matching what MyST permits. + + An author with no name is dropped, with a warning: Zenodo requires a name + on every creator, so including one would fail the entire deposit rather + than lose the one entry. Junk input yields an empty list -- this function + must never itself be the reason a deposit fails. + + The caller's authors are left untouched. + """ + if not isinstance(authors, (list, tuple)): + return [] + + creators = [] + for author in authors: + if not isinstance(author, dict): + if _is_blank(author): + continue + creators.append({"name": str(author).strip()}) + continue + + creator = {} + repaired = {} + for key, value in author.items(): + if _is_blank(value): + continue + field = _canonical_creator_field(key) + if field is None: + continue + target = creator if str(key).strip().lower() == field else repaired + target.setdefault(field, value) + + # An exactly-named key is authoritative; a repaired one only fills a + # field the author did not spell correctly anywhere. + for field, value in repaired.items(): + creator.setdefault(field, value) + + if not creator.get("name"): + logging.warning( + f"Skipping an author with no name in the Zenodo creator list: " + f"{author!r}." + ) + continue + + creators.append({ + field: value if isinstance(value, str) else str(value) + for field, value in creator.items() + }) + + return creators diff --git a/systemd/neurolibre-mystbuild-firewall.service b/systemd/neurolibre-mystbuild-firewall.service new file mode 100644 index 0000000..811d53d --- /dev/null +++ b/systemd/neurolibre-mystbuild-firewall.service @@ -0,0 +1,30 @@ +[Unit] +# Blocks the OpenStack instance metadata service (169.254.169.254) for +# containers on the myst build network. Build containers execute notebook code +# from submitted repositories; metadata serves user-data and injected +# credentials, so it must not be reachable from them. +# +# Docker has no per-container egress ACL, hence a host rule. DOCKER-USER is +# processed before Docker's own chains and matches forwarded traffic only, so +# the host's own metadata access (cloud-init) is unaffected. +# +# Requires the build network to exist with a fixed bridge name: +# docker network create --driver bridge \ +# --opt com.docker.network.bridge.name=br-mystbuild mystbuild +# +# Do NOT use iptables-persistent for this. It snapshots the entire ruleset, +# including Docker's generated rules, and restoring those at boot before Docker +# starts causes duplicated and conflicting rules. +Description=Block instance metadata for myst build containers +After=docker.service +Requires=docker.service + +[Service] +Type=oneshot +RemainAfterExit=yes +# -C tests for the rule first, so restarting the unit cannot stack duplicates +ExecStart=/bin/sh -c '/sbin/iptables -C DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT 2>/dev/null || /sbin/iptables -I DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT' +ExecStop=/bin/sh -c '/sbin/iptables -D DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT 2>/dev/null || true' + +[Install] +WantedBy=multi-user.target diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py new file mode 100644 index 0000000..89035d3 --- /dev/null +++ b/tests/test_myst_frontmatter.py @@ -0,0 +1,375 @@ +import json + +import pytest + +from api.myst_frontmatter import myst_project_metadata +from api.myst_frontmatter import merge_paper_metadata +from api.myst_frontmatter import first_affiliations + +FRONT_MATTER_PAPER = """--- +title: Front Matter Title +authors: + - name: Ada Lovelace + affiliation: "1" +affiliations: + - name: Analytical Engine Institute + index: 1 +--- + +Body. +""" + +AUTHORS_ONLY_PAPER = """--- +authors: + - name: Ada Lovelace + affiliation: "1" +--- + +Body. +""" + +MYST_YML = """project: + title: Myst Title + date: "02 February 2022" + keywords: + - myst keyword + authors: + - name: Grace Hopper + affiliations: society + affiliations: + - id: society + institution: Royal Society +""" + +MALFORMED_MYST_YML = 'project:\n title: "unterminated\n authors: [ {\n' + + +def test_front_matter_wins_over_myst_yml(): + data = merge_paper_metadata( + {"title": "Front Matter Title", + "authors": [{"name": "Ada Lovelace", "affiliation": "1"}], + "affiliations": [{"index": 1, "name": "Analytical Engine Institute"}]}, + MYST_YML, + ) + assert data["title"] == "Front Matter Title" + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + assert data["affiliations"] == [{"index": 1, "name": "Analytical Engine Institute"}] + + +def test_myst_yml_fills_a_paper_with_no_front_matter(): + data = merge_paper_metadata(None, MYST_YML) + assert data["title"] == "Myst Title" + assert data["authors"][0]["name"] == "Grace Hopper" + assert data["authors"][0]["affiliation"] == "1" + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_scalar_fields_fill_individually(): + data = merge_paper_metadata({"title": "Kept"}, MYST_YML) + assert data["title"] == "Kept" + assert data["date"] == "02 February 2022" + assert data["tags"] == ["myst keyword"] + + +def test_authors_and_affiliations_are_filled_as_a_pair(): + # The front matter has authors but no affiliations, so both must come from + # myst.yml rather than pairing index 1 with the wrong institution. + data = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]}, MYST_YML + ) + assert [a["name"] for a in data["authors"]] == ["Grace Hopper"] + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_returns_none_when_neither_source_has_authors(): + assert merge_paper_metadata(None, None) is None + assert merge_paper_metadata({"title": "Only a title"}, None) is None + + +def test_tolerates_a_malformed_myst_yml(): + data = merge_paper_metadata( + {"title": "Front Matter Title", + "authors": [{"name": "Ada Lovelace", "affiliation": "1"}], + "affiliations": [{"index": 1, "name": "Analytical Engine Institute"}]}, + MALFORMED_MYST_YML, + ) + assert data["title"] == "Front Matter Title" + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + + +def test_tolerates_a_myst_yml_with_no_project_key(): + data = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace"}]}, "site:\n title: Not a project\n" + ) + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + + +def test_does_not_mutate_the_caller_s_front_matter(): + front_matter = {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]} + merge_paper_metadata(front_matter, MYST_YML) + assert front_matter == {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]} + + +def test_composes_affiliation_name_from_parts_in_order(): + project = { + "affiliations": [ + { + "id": "full", + "department": "Département de génie physique", + "institution": "École Polytechnique de Montréal", + "address": "2500 Chemin de Polytechnique", + "city": "Montreal", + "region": "Quebec", + "postal_code": "H3T 1J4", + "country": "Canada", + } + ] + } + assert myst_project_metadata(project)["affiliations"] == [ + { + "index": 1, + "name": ( + "Département de génie physique, " + "École Polytechnique de Montréal, " + "2500 Chemin de Polytechnique, " + "Montreal, Quebec, H3T 1J4, Canada" + ), + } + ] + + +def test_honours_name_and_state_aliases(): + project = { + "affiliations": [{"id": "a", "name": "Harvard University", "state": "Massachusetts"}] + } + assert myst_project_metadata(project)["affiliations"] == [ + {"index": 1, "name": "Harvard University, Massachusetts"} + ] + + +def test_resolves_affiliation_ids_to_indices(): + project = { + "authors": [ + {"name": "Ada Lovelace", "affiliations": ["engine", "society"]}, + {"name": "Grace Hopper", "affiliations": "society; engine"}, + ], + "affiliations": [ + {"id": "engine", "institution": "Analytical Engine Institute"}, + {"id": "society", "institution": "Royal Society"}, + ], + } + result = myst_project_metadata(project) + assert [a["affiliation"] for a in result["authors"]] == ["1,2", "2,1"] + + +def test_appends_undeclared_affiliation_id_as_literal_name(): + project = { + "authors": [{"name": "Grace Hopper", "affiliations": "Yale University"}], + "affiliations": [{"id": "engine", "institution": "Analytical Engine Institute"}], + } + result = myst_project_metadata(project) + assert result["authors"][0]["affiliation"] == "2" + assert result["affiliations"][1] == {"index": 2, "name": "Yale University"} + + +def test_maps_author_fields(): + project = { + "authors": [ + { + "name": "Ada Lovelace", + "email": "ada@example.org", + "orcid": "0000-0002-1825-0097", + "corresponding": True, + "equal_contributor": True, + } + ] + } + author = myst_project_metadata(project)["authors"][0] + assert author["email"] == "ada@example.org" + assert author["orcid"] == "0000-0002-1825-0097" + assert author["corresponding"] is True + assert author["equal-contrib"] is True + + +def test_maps_scalar_fields(): + project = { + "title": "T", + "date": "03 March 2023", + "keywords": ["photon counting"], + "bibliography": ["content/paper.bib"], + } + result = myst_project_metadata(project) + assert result["title"] == "T" + assert result["date"] == "03 March 2023" + assert result["tags"] == ["photon counting"] + assert result["bibliography"] == ["content/paper.bib"] + + +def test_does_not_map_doi_license_or_venue(): + project = {"doi": "10.55458/neurolibre.xxxxx", "license": {"content": "CC-BY-4.0"}, "venue": "Neurolibre"} + assert myst_project_metadata(project) == {} + + +def test_author_without_affiliations_gets_no_affiliation_key(): + project = {"authors": [{"name": "Ada Lovelace"}]} + assert "affiliation" not in myst_project_metadata(project)["authors"][0] + + +@pytest.mark.parametrize("project", [None, {}, "not a mapping", []]) +def test_tolerates_junk_input(project): + assert myst_project_metadata(project) == {} + + +AFFILIATIONS = [ + {"index": 1, "name": "Analytical Engine Institute"}, + {"index": 2, "name": "Royal Society"}, +] + + +def test_first_affiliations_resolves_a_single_index(): + authors = [{"name": "Ada Lovelace", "affiliation": "1"}] + assert first_affiliations(authors, AFFILIATIONS) == ["Analytical Engine Institute"] + + +def test_first_affiliations_takes_the_first_of_a_comma_string(): + authors = [{"name": "Grace Hopper", "affiliation": "2,1"}] + assert first_affiliations(authors, AFFILIATIONS) == ["Royal Society"] + + +def test_first_affiliations_accepts_an_int(): + authors = [{"name": "Ada Lovelace", "affiliation": 2}] + assert first_affiliations(authors, AFFILIATIONS) == ["Royal Society"] + + +def test_first_affiliations_is_none_when_the_key_is_absent(): + authors = [{"name": "The Analytical Collaboration"}] + assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_is_none_for_an_empty_string(): + authors = [{"name": "The Analytical Collaboration", "affiliation": ""}] + assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_is_none_for_an_undeclared_index(): + authors = [{"name": "Ada Lovelace", "affiliation": "9"}] + assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_warns_about_an_undeclared_index(caplog): + authors = [{"name": "Ada Lovelace", "affiliation": "9"}] + with caplog.at_level("WARNING"): + assert first_affiliations(authors, AFFILIATIONS) == [None] + assert "Ada Lovelace" in caplog.text + assert "9" in caplog.text + + +def test_first_affiliations_tolerates_an_empty_affiliation_list(): + # A front matter with authors only, plus a myst.yml project that names no + # authors, reaches the deposit task with authors and no affiliations. + authors = [{"name": "Ada Lovelace", "affiliation": "1"}, {"name": "Grace Hopper"}] + assert first_affiliations(authors, []) == [None, None] + + +UNQUOTED_DATE_MYST_YML = """project: + date: 2024-01-15 + authors: + - name: Grace Hopper +""" + + +def test_unquoted_iso_date_maps_to_a_string(): + # yaml.safe_load turns an unquoted ISO date into a datetime.date. + data = merge_paper_metadata(None, UNQUOTED_DATE_MYST_YML) + assert data["date"] == "2024-01-15" + assert isinstance(data["date"], str) + + +def test_metadata_from_an_unquoted_date_is_json_serialisable(): + # The real failure mode: the metadata becomes a Celery task payload, and + # Celery serialises tasks as JSON. + data = merge_paper_metadata(None, UNQUOTED_DATE_MYST_YML) + assert json.loads(json.dumps(data))["date"] == "2024-01-15" + + +@pytest.mark.parametrize("blank", [None, "", [], {}]) +def test_blank_front_matter_keys_are_filled_from_myst_yml(blank): + data = merge_paper_metadata( + {"title": blank, "authors": blank, "affiliations": blank}, MYST_YML + ) + assert data["title"] == "Myst Title" + assert [a["name"] for a in data["authors"]] == ["Grace Hopper"] + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_bare_string_authors_become_named_authors(): + project = {"authors": ["Ada Lovelace", "Grace Hopper"]} + authors = myst_project_metadata(project)["authors"] + assert [a["name"] for a in authors] == ["Ada Lovelace", "Grace Hopper"] + assert all("affiliation" not in author for author in authors) + + +MIXED_AFFILIATIONS_PROJECT = { + "authors": [{"name": "Ada Lovelace", "affiliations": "b"}], + "affiliations": [ + {"id": "a", "institution": "Alpha University"}, + "Bare String Institute", + {"id": "b", "institution": "Beta University"}, + ], +} + + +def test_a_bare_string_affiliation_consumes_its_index_position(): + result = myst_project_metadata(MIXED_AFFILIATIONS_PROJECT) + assert result["affiliations"] == [ + {"index": 1, "name": "Alpha University"}, + {"index": 2, "name": "Bare String Institute"}, + {"index": 3, "name": "Beta University"}, + ] + assert result["authors"][0]["affiliation"] == "3" + + +def test_a_scalar_affiliations_value_is_one_affiliation(): + # `affiliations: harvard` is legal MyST; iterating the string would yield + # one affiliation per character. + result = myst_project_metadata( + {"authors": [{"name": "Ada Lovelace"}], "affiliations": "Harvard University"} + ) + assert result["affiliations"] == [{"index": 1, "name": "Harvard University"}] + + +def test_a_scalar_authors_value_is_one_author(): + result = myst_project_metadata({"authors": "Ada Lovelace"}) + assert [a["name"] for a in result["authors"]] == ["Ada Lovelace"] + + +def test_first_affiliations_tolerates_a_malformed_affiliation_entry(): + # A hand-written paper.md may omit `index` or `name` on an entry. That is a + # typo in one entry, not a reason to fail the whole deposit. + authors = [{"name": "Ada Lovelace", "affiliation": "1"}] + affiliations = [{"name": "No Index Institute"}, {"index": 1}] + assert first_affiliations(authors, affiliations) == [None] + + +def test_first_affiliations_strips_whitespace_around_an_index(): + authors = [{"name": "Ada Lovelace", "affiliation": " 1 , 2"}] + affiliations = [{"index": 1, "name": "Analytical Engine Institute"}] + assert first_affiliations(authors, affiliations) == [ + "Analytical Engine Institute" + ] + + +def test_first_affiliations_tolerates_a_bare_string_author(): + # `authors: [Ada Lovelace]` is legal in both sources. + assert first_affiliations(["Ada Lovelace"], []) == [None] + + +def test_warns_when_myst_yml_authors_replace_front_matter_authors(caplog): + # Authors and affiliations are filled as a pair, so a front matter that + # names authors but no affiliations loses its author list entirely. Say so. + with caplog.at_level("WARNING"): + metadata = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace"}]}, MYST_YML + ) + assert metadata["authors"][0]["name"] == "Grace Hopper" + assert "replacing" in caplog.text.lower() diff --git a/tests/test_zenodo_metadata.py b/tests/test_zenodo_metadata.py new file mode 100644 index 0000000..3d50034 --- /dev/null +++ b/tests/test_zenodo_metadata.py @@ -0,0 +1,132 @@ +"""What reaches a Zenodo deposit, and what must not.""" + +import json + +import pytest + +from api.zenodo_metadata import ZENODO_CREATOR_FIELDS +from api.zenodo_metadata import zenodo_creators + + +def test_keeps_only_the_fields_zenodo_accepts(): + creators = zenodo_creators([ + { + "name": "Ada Lovelace", + "orcid": "0000-0001-0000-0000", + "affiliation": "Analytical Engine Institute", + } + ]) + assert creators == [{ + "name": "Ada Lovelace", + "orcid": "0000-0001-0000-0000", + "affiliation": "Analytical Engine Institute", + }] + + +def test_drops_the_author_email(): + # myst.yml authors routinely carry an email; paper.md front matter rarely + # does. Zenodo rejects the key, and a public record must not publish it. + creators = zenodo_creators([ + {"name": "Ada Lovelace", "email": "ada@example.org"} + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_drops_the_myst_only_author_keys(): + creators = zenodo_creators([{ + "name": "Ada Lovelace", + "email": "ada@example.org", + "corresponding": True, + "equal-contrib": True, + "github": "ada", + "twitter": "ada", + "url": "https://example.org", + "numbering": {"heading_1": False}, + }]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_repairs_a_misspelled_orcid_key(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orchid": "0000-0001-0000-0000"} + ]) + assert creators == [ + {"name": "Ada Lovelace", "orcid": "0000-0001-0000-0000"} + ] + + +def test_repairs_a_plural_affiliation_key(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliations": "Royal Society"} + ]) + assert creators == [ + {"name": "Ada Lovelace", "affiliation": "Royal Society"} + ] + + +def test_an_exact_field_wins_over_a_repaired_one(): + # Deterministic regardless of dict order: the exact key is authoritative. + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliations": "Wrong", "affiliation": "Right"} + ]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "Right"}] + + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliation": "Right", "affiliations": "Wrong"} + ]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "Right"}] + + +def test_drops_blank_values(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orcid": None, "affiliation": ""} + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_a_bare_string_author_becomes_a_named_creator(): + assert zenodo_creators(["Ada Lovelace"]) == [{"name": "Ada Lovelace"}] + + +def test_stringifies_a_non_string_scalar(): + creators = zenodo_creators([{"name": "Ada Lovelace", "affiliation": 1}]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "1"}] + + +def test_skips_an_author_with_no_name(): + # Zenodo requires a name on every creator. Sending one without would fail + # the whole deposit; dropping it loses one creator instead of all of them. + creators = zenodo_creators([ + {"orcid": "0000-0001-0000-0000"}, + {"name": "Ada Lovelace"}, + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_warns_about_an_author_with_no_name(caplog): + with caplog.at_level("WARNING"): + zenodo_creators([{"orcid": "0000-0001-0000-0000"}]) + assert "no name" in caplog.text + + +def test_tolerates_junk_input(): + assert zenodo_creators(None) == [] + assert zenodo_creators([]) == [] + assert zenodo_creators("not a list") == [] + + +def test_does_not_mutate_the_caller_s_authors(): + authors = [{"name": "Ada Lovelace", "email": "ada@example.org"}] + zenodo_creators(authors) + assert authors == [{"name": "Ada Lovelace", "email": "ada@example.org"}] + + +def test_result_is_json_serialisable(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orcid": "0000-0001-0000-0000"} + ]) + assert json.loads(json.dumps(creators)) == creators + + +def test_allowed_fields_are_the_zenodo_legacy_creator_schema(): + assert ZENODO_CREATOR_FIELDS == ("name", "affiliation", "orcid", "gnd")