Skip to content

Pilot: enable RLS on score_calibrations in staging #825

Description

@bencap

Enable RLS on one table in staging before touching the rest. score_calibrations is the right choice: smallest surface, strictest rule, the entity that leaked in #805 and #807, and the one where the RETURNING pitfall below is most likely to surface.

Scope

Enable RLS and attach the score_calibrations policies in staging. Leave the hand-written has_permission filters in place for this phase — they become redundant, not wrong, so the pilot is reversible by dropping the policies.

The policy is not a delegation. score_calibrations carries a rule of its own, conditional on a local column and reaching three tables:

ALTER TABLE score_calibrations ENABLE ROW LEVEL SECURITY;
ALTER TABLE score_calibrations FORCE ROW LEVEL SECURITY;

CREATE POLICY score_calibrations_select ON score_calibrations FOR SELECT USING (
  private = false
  OR created_by_id = nullif(current_setting('app.user_id', true), '')::integer
  OR (investigator_provided AND EXISTS (
        SELECT 1 FROM scoresets ss
         WHERE ss.id = score_calibrations.score_set_id
           AND (ss.created_by_id = nullif(current_setting('app.user_id', true), '')::integer
                OR ss.id IN (SELECT sc.scoreset_id
                               FROM scoreset_contributors sc
                               JOIN contributors c ON c.id = sc.contributor_id
                              WHERE c.orcid_id = (SELECT u.username FROM users u
                                                   WHERE u.id = nullif(current_setting('app.user_id', true), '')::integer)))
     ))
  OR EXISTS (SELECT 1 FROM users_roles ur JOIN roles r ON r.id = ur.role_id
              WHERE ur.user_id = nullif(current_setting('app.user_id', true), '')::integer
                AND r.name = ANY (nullif(current_setting('app.active_roles', true), '')::text[])
                AND r.name IN ('admin'))
);

CREATE POLICY score_calibrations_system ON score_calibrations FOR ALL
  TO mavedb_system USING (true) WITH CHECK (true);

score_set_id IN (SELECT id FROM scoresets) is not a valid substitute for the third disjunct. A public score set is visible to everyone, so it would expose a private investigator-provided calibration to anonymous callers.

Requires the modified_by_id removal and the score_calibrations.score_set_id index from #809.

What the pilot tests

  • INSERT ... RETURNING — Postgres requires the new row to satisfy the SELECT policy, and SQLAlchemy uses RETURNING to fetch primary keys. The created_by_id disjunct should make this safe, but calibration is where the rule differs most from its parent.
  • Silent zero-row writes — every calibration write path still affects rows. This is the failure mode with no error attached.
  • The leak case directly — a private investigator-provided calibration on a public score set is invisible to an anonymous caller and to a caller who is neither its creator nor a contributor to the score set.
  • Matview safetypublished_variants_materialized_view is unaffected. RLS never applies to materialized views, so its published_date IS NOT NULL filter is load-bearing and needs a test here.
  • Primary-calibration detection still sees rows the caller cannot, via the SECURITY DEFINER helper.
  • Plan quality on the calibration read paths, compared against the same queries with policies dropped. The nested scoresets reference is the new cost; confirm it does not turn a narrow lookup into a scan.

Acceptance criteria

Metadata

Metadata

Assignees

No one assigned

    Labels

    app: backendTask implementation touches the backend

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions