Skip to content

Add read-only privacy query command - #547

Open
Inengs wants to merge 2 commits into
chaoss:mainfrom
Inengs:feat/privacy-query
Open

Inengs wants to merge 2 commits into
chaoss:mainfrom
Inengs:feat/privacy-query

Conversation

@Inengs

@Inengs Inengs commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Description
Adds collectoss privacy query EMAIL [-o FILE] [--schema TEXT], a read-only command that reports the personal data an instance holds about one person.

  • Resolves the email once to other emails, platform logins and contributor ids, then reports matching rows from 18 tables as JSON.
  • No schema changes. Removal is out of scope for this PR.
  • Logic is in collectoss/application/privacy.py, CLI in collectoss/application/cli/privacy.py, tests in tests/test_application/test_cli/test_privacy_query.py.

Part of #523
Notes for Reviewers

  • Tested with 13 unit tests (SQLite, made-up people). The full suite passes (130).
  • Also run against a freshly migrated Postgres 16 (migration 45). Every table and column in the registry exists in schema data, and a fake contributor is found end to end.
  • Use -o FILE to save a report (written with mode 0600). stdout also receives the existing "Connectivity test" line from test_connection, so redirecting stdout does not give valid JSON.
  • Known limitation: non-commit rows can carry another person's contributor id (e.g. issue reporter or assignee). Only commits are trimmed so far. This is listed in the report's limitations.
  • Bandit reports 5 B608 warnings. Schema, table and column names come from a fixed registry and are validated by _ident. Values are bound parameters. I'm happy to rewrite it with SQLAlchemy Core if you prefer.
  • I'm open to moving the module if you'd rather have it somewhere else.

Signed commits

  • Yes, I signed my commits.

Generative AI disclosure

  • This contribution was NOT assisted or created by Generative AI tools.
  • This contribution was assisted or created by Generative AI tools.

If AI tools were used, please provide details below:
- What tools were used? Claude (Claude Code)
- How were these tools used? Drafting the module, CLI and tests, based on the design agreed in the epic. Also used for explaining the codebase and reviewing my test results.
- Did you review these outputs before submitting this PR? Yes. I made sure it was coherent with existing codebase patterns, I made sure to test properly, I also walked through some parts of the codes and made tweeks.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X2DzVeoy7oZojjFABexuFf

@Inengs
Inengs requested a review from MoralCode as a code owner October 17, 2026 12:11
Comment thread collectoss/application/privacy.py Fixed
Comment thread collectoss/application/privacy.py Fixed
Comment thread collectoss/application/privacy.py Fixed
Comment thread collectoss/application/privacy.py Fixed
Comment thread collectoss/application/privacy.py Fixed
@MoralCode

Copy link
Copy Markdown
Contributor

Yeah, I'd like to move away from manually assembling SQL queries where possible, bandit is complaining about potential SQL injection. Utilizing sqlalchemy more would be great but avoiding injection is the main issue.

Github mobile is being annoying so I'll leave some more thoughts here:

  • I'm curious why you chose to build a whole new type (Source) to list out all the tables that need checking too.
  • I'd also love to try and reuse functions where possible for functionality like validating email addresses, or at least have them In a central place.
  • traditionally this codebase has put database-accessing functions in application/db/lib.py. maybe we can follow a similar pattern by making a group of functions in there for doing the database access parts of this?
  • sqlite has a very different feature set/list of supported data types etc than postgreSQL, I think it'd be preferable to test this on real data after letting CollectOSS gather some data on one of its default repos (although automating this requires a lot of testing infrastructure we dont yet have fully set up)
  • I dont think we have to worry too much about finding PII thats in the contents of a comment, especially if not posted by the user being looked up

I'm also realizing this is a much bigger change than I thought it was when I suggested having the check/export in one step.

I think maybe if we can get the internal infrastructure in place to query all data tied to a person (maybe sqlalchemy can help traverse the foreign keys for us???) Figured out first, adding a CLI layer on top of that will be relatively easier than trying both at once.

Also you point out a valid issue with the CLI connection test printing and messing with any ability to pipe from the CLI. IMO that should be opened as a new issue so we can resolve that and figure out how best to support outputting data (and whether the CLI output is considered a stable api surface or not)

@Inengs

Inengs commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks, this all makes sense.

I’ll rework this: drop Source, use the model metadata to find tables referencing contributors.cntrb_id plus a short list of plain-text email columns, use SQLAlchemy select() instead of SQL strings, and put the DB access in application/db/lib.py. I’ll drop the CLI from this PR and open it as a follow-up, and verify on Postgres with real collected data instead of SQLite.

I believe that once i use sqlAlchemy, I wont need to manually assemble sql queries, and that will remove the bandit error.

The use of Source was just a wrong implementation decision, I felt it will be faster just listing out the tables and columns to check, but now I will use sql alchemy' model metadata to find tables that are connected to the foreign key

I will open a new issue for the connectivity test message issue, and link it here

Inengiye Emmanuel added 2 commits October 20, 2026 09:47
Adds 'collectoss privacy query EMAIL [-o FILE] [--schema TEXT]', which reports the personal data an instance holds about one person. No schema changes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01X2DzVeoy7oZojjFABexuFf
Signed-off-by: Inengiye Emmanuel <inengiyeeemmanuel@gmail.com>
Add functions in db/lib.py that find every row tied to a person,
starting from an email. Read-only, built with SQLAlchemy select().
Contributor references are found by walking foreign keys; plain-text
email columns are listed explicitly, with a test that fails if a new
email column is added without being covered.

Replaces the earlier CLI-based approach; the CLI layer will follow.

Signed-off-by: Inengiye Emmanuel <inengiyeeemmanuel@gmail.com>
@Inengs
Inengs force-pushed the feat/privacy-query branch from 4027f12 to f55d969 Compare October 20, 2026 08:47
@Inengs

Inengs commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I've reworked the PR along those lines and force-pushed.

What changed

  • Dropped the CLI layer and the Source registry type. This PR is now internal infrastructure only, in application/db/lib.py, with a shared normalize_email in util/validation.py.
  • Queries use SQLAlchemy select(). No hand-built SQL, and Bandit reports nothing in the new code (the three remaining findings in lib.py were already there).
  • Contributor references are found by walking the model metadata foreign keys to contributors.cntrb_id / cntrb_login, so new tables that reference a contributor are picked up automatically.
  • get_person_filters() returns one WHERE clause per table. A SELECT uses it now, and a removal feature can run a DELETE with the same clauses, so the two can't drift apart.
  • Tests are DB-free and read only the model metadata. One of them fails if a new email column is added without being searched or explicitly excluded. That test found nine columns my first list missed (the dm_repo_* tables, unknown_cache, exclude, spdx.creators).

Known limits

  • Committers are matched by email only: commits.cmt_ght_committer_id has no FK and no collector writes it.
  • release_author is matched only when it ends in _<email> (tag-only releases). The name_company form isn't matched.
  • Rows are returned whole, so a shared row can include another person's id or email.
  • Alias chains are expanded once, not followed repeatedly.
  • dm_repo_* and unknown_cache are rebuilt from commits by facade, so deleting rows there won't stick while commits remain. Relevant for the removal epic.
  • Per-email lower(col) IN (...) can't use plain indexes. I haven't measured it on a large instance.

Not covered, for discussion

  • operations.users (email, phone, name) is a separate identity with no link to contributors, and its neighbouring tables hold secrets (tokens, API keys, password hash). I excluded it deliberately and think it needs its own design with a column allow-list.
  • rgls_email and chaoss_email are also excluded for now. Do you want them in scope?

Testing
I'm running this against real collected data on a small repo now and will post counts, a cross-check against plain SQL, and EXPLAIN ANALYZE results here.

The CLI connectivity output going to stdout is filed separately as #<548>. The CLI layer will be a follow-up PR on top of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants