fix: Permission narrowing within entities - #805
Open
bencap wants to merge 6 commits into
Open
Conversation
…iewer A calibration's READ rule is stricter than its score set's: publishing a score set does not publish its calibrations, and reading one does not entitle a caller to read the private calibrations hanging off it. Reads that fanned out from a score set to its calibrations left the router's assert_permission boundary behind and served every calibration they found. Introduce Principal and the generic Viewer, so "may this caller see this?" can be asked at the point of fan-out rather than assumed to have been asked upstream. Both default to anonymous, so a caller that constructs one with no argument serves the public view rather than everything in the database. Apply it to the paths that fan out today: VA-Spec annotation building, the recently-published listing, and the public data export. Annotations are emitted per viewer rather than public-only, so an entitled caller still receives evidence drawn from calibrations they may read. Because a VA-Spec statement carries no stable identifier, every emitted annotation now discloses its own scope through a mavedb_calibration_scope extension, so a consumer can tell whether the record is the one anyone would receive or one widened by the requester's access.
…core-set listing
GET /experiments/{urn}/score-sets returns the full score set view model,
which carries score_calibrations, but applied no calibration filter. A
published score set can hold an unpublished calibration, so an anonymous
caller listing an experiment's score sets received every private
calibration's baseline score, threshold ranges, oddsPath ratios and ACMG
criteria.
Filter through ScoreCalibrationViewer, the same rule the score set
detail and recently-published endpoints use.
The filter is applied to the serialized view rather than by reassigning
ScoreSet.score_calibrations. That relationship is mapped with
cascade="all, delete-orphan", so narrowing the ORM collection marks the
withheld rows as orphans and the next flush deletes them; read paths
avoid that today only because the session is built with autoflush=False
and no read handler commits.
The export narrowed its data by assigning filtered lists back onto ExperimentSet.experiments and ScoreSet.score_calibrations. Both relationships are mapped with cascade="all, delete-orphan", so those assignments did not merely shape the dump -- they marked every withheld row as an orphan. This script can flush: with_database_session commits when the command is invoked with --commit, at which point the withheld private calibrations are deleted, along with every published experiment carrying no CC0 score sets and, by cascade, its score sets and variants. Only --dry-run, the default, made this survivable. Validate the experiment sets first, then narrow the resulting ExperimentSetPublicDump views. Pydantic models are throwaway response objects, so narrowing them stages nothing. The ORM graph is now only read: visible calibration ids are collected in a read-only pass and the views are filtered by id. Score set ids for the per-score-set files are now taken from the narrowed views rather than the ORM graph, so the csv/, mapped/ and va/ entries cover exactly what main.json describes.
A calibration's READ rule is stricter than its score set's in both directions: publishing a score set does not publish its calibrations, and owning a score set does not entitle its owner to a community calibration -- one contributed by a non-contributor -- attached to it. The owner-facing endpoints returned the score set wholesale, so creating, updating, uploading to or publishing a score set handed its owner calibrations they cannot fetch directly. Route every ScoreSet response in this module through _score_set_response, which narrows both sub-resources whose rules diverge from the score set's own: calibrations, and the superseding score set. The search routes are the documented exception -- they answer with ShortScoreSet, which carries neither. Centralizing this removes the last four ORM-mutation filters here. ScoreSet.score_calibrations cascades delete-orphan, so narrowing it in place marked the withheld rows as orphans; assigning superseding_score_set = None nulls the other score set's replaces_id, but only once the attribute has been read, which the permission check did immediately before. fetch_score_set_by_urn now returns the score set as it is, which is also what its non-response callers -- supersession lookup and publication -- actually want. _score_set_response is module-private deliberately. A shared response constructor would not cover the CSV, VA-Spec NDJSON or public-dump serializations of the same graph, so the durable fix belongs at the session rather than the response layer.
…e ORM fetch_experiment_set filtered its response by assigning into item.experiments in place. ExperimentSet.experiments is mapped with cascade="all, delete-orphan", so that assignment marked every experiment the caller could not read as an orphan, and any subsequent flush would have deleted those experiments along with their score sets and variants. Verified: the assignment plus a commit drops the experiment count. Nothing flushed on this path, so nothing was lost -- SessionLocal is built autoflush=False and the handler never commits. Both are coincidences rather than guarantees. Build a local list instead, and sort that rather than the ORM collection.
bencap
marked this pull request as ready for review
August 6, 2026 23:11
Coverage Report for CI Build 31130112892Warning No base build found for commit Coverage: 89.531%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
9 tasks
This was referenced Aug 7, 2026
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
2 participants
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.