Make sources.filesystem_id non-nullable #6350

legoktm · 2022-03-18T19:13:24Z

Status

Ready for review

Description of Changes

We already set this field all the time and in all places expect it to be
a string, not checking if its None first.

Discovered by the mypy work I'm doing in #6346.

Refs #6226.

Testing

Before applying this patch, create a source, log in with it. Then check out this branch, in the dev container run alembic upgrade head to apply this schema change, and try to log in with the source. Verify you can log in properly.
CI passes

Deployment

Any special considerations for deployment? No, just a normal schema change.

Checklist

Linting (make lint) and tests (make test) pass in the development container
I have written a test plan and validated it for this PR
These changes do not require documentation

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

codecov-commenter · 2022-03-21T21:52:07Z

Codecov Report

Merging #6350 (fc72191) into develop (3e39d73) will decrease coverage by 0.04%.
The diff coverage is 69.23%.

@@             Coverage Diff             @@
##           develop    #6350      +/-   ##
===========================================
- Coverage    84.00%   83.95%   -0.05%     
===========================================
  Files           61       62       +1     
  Lines         4295     4307      +12     
  Branches       522      522              
===========================================
+ Hits          3608     3616       +8     
- Misses         564      568       +4     
  Partials       123      123

Impacted Files	Coverage Δ
...ns/b7f98cfd6a70_make_filesystem_id_non_nullable.py	`66.66% <66.66%> (ø)`
securedrop/models.py	`89.57% <100.00%> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

legoktm · 2022-03-24T20:56:48Z

I tried to git blame on whether we were ever intentionally writing NULLs to filesystem_id and didn't really find anything (but also the filesystem_id column dates back to the initial SQLAlchemy commit, so it's super old!).

I am relatively confident that there aren't NULLs lying around because SD would blow up pretty quickly in the app code if filesystem_id was NULL, so moving this to ready for review.

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

zenmonkeykstop · 2022-03-31T15:15:13Z

Poked around a bit as well (including manually setting a source's filesystem_id to null and seeing what happens) - returning users are authenticated by generating the expected filesystem_id and querying the database for it. So, if a source's filesystem_id field is ever set to null, they can't log back in.

Probably what this means for this PR is that just deleting sources with a null filesystem_id value is a valid migration strategy, as their submissions are already effectively orphaned and will get swept up in the nightly jobs to detect disconnected files/entries.

legoktm · 2022-03-31T20:02:54Z

Deleting a source is not the easiest thing in alembic because we need to clean up referential tables, but it seems doable.

legoktm · 2022-03-31T21:43:00Z

Moving back to draft state, I pushed my work from today but there's still a bit more to do in the migration, we delete stuff out of sources, replies, submissions, but aren't then deleting stuff that refer to those replies and submissions...

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

We already set this field all the time and in all places expect it to be a string, not checking if its None first. The database migration will delete any sources that have a NULL filesystem_id, plus anything that references those sources. Discovered by the mypy work I'm doing in #6346. Refs #6226.

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

zenmonkeykstop

Tested as follows:

set up 2.3.1 prod VM environment, created multiple sources, nulled out the filesystem ID for one of them.
Confirmed that a 500 error is thrown on the All Sources page due to the missing filesystem_id (so this unlikely scenario would have been immediately noticeable)
used the upgrade scenario procedure to update the prod instance with debs built from this branch
confirmed that the All Sources page no longer 500s out and displays only the non-nulled sources.
confirmed that replies and message from those sources are still available for download
confirmed that the nulled source was deleted from the database
confirmed that the filesystem_id field has nullable=false applied.

LGTM!

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

This was referenced Mar 18, 2022

Upgrade mypy and enable SQLAlchemy plugin #6351

Merged

Next schema change will fail test_schema_unchanged_after_up_then_downgrade because of instance_config weirdness #6352

Closed

legoktm force-pushed the filesystem-id-non-null branch from 870ebcc to fc72191 Compare March 21, 2022 21:29

legoktm marked this pull request as ready for review March 24, 2022 20:56

legoktm requested a review from a team as a code owner March 24, 2022 20:57

legoktm added this to Ready for Review in SecureDrop Team Board Mar 24, 2022

eloquence assigned zenmonkeykstop Mar 30, 2022

legoktm force-pushed the filesystem-id-non-null branch from fc72191 to 9725557 Compare March 31, 2022 21:41

legoktm marked this pull request as draft March 31, 2022 21:41

legoktm force-pushed the filesystem-id-non-null branch 2 times, most recently from f5d52e6 to 27515a2 Compare April 1, 2022 04:29

legoktm marked this pull request as ready for review April 1, 2022 04:39

legoktm force-pushed the filesystem-id-non-null branch 2 times, most recently from a160a6b to 0fb2dd9 Compare April 7, 2022 22:12

legoktm force-pushed the filesystem-id-non-null branch from 0fb2dd9 to 9d1efc8 Compare April 12, 2022 16:54

zenmonkeykstop approved these changes Apr 19, 2022

View reviewed changes

zenmonkeykstop merged commit b5810d3 into develop Apr 19, 2022

SecureDrop Team Board automation moved this from Ready for Review to Done Apr 19, 2022

legoktm deleted the filesystem-id-non-null branch April 19, 2022 15:16

zenmonkeykstop mentioned this pull request May 10, 2022

Release SecureDrop 2.4.0 #6440

Closed

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sources.filesystem_id non-nullable #6350

Make sources.filesystem_id non-nullable #6350

legoktm commented Mar 18, 2022 •

edited

codecov-commenter commented Mar 21, 2022 •

edited

legoktm commented Mar 24, 2022

zenmonkeykstop commented Mar 31, 2022

legoktm commented Mar 31, 2022

legoktm commented Mar 31, 2022

zenmonkeykstop left a comment

Make sources.filesystem_id non-nullable #6350

Make sources.filesystem_id non-nullable #6350

Conversation

legoktm commented Mar 18, 2022 • edited

Status

Description of Changes

Testing

Deployment

Checklist

codecov-commenter commented Mar 21, 2022 • edited

Codecov Report

legoktm commented Mar 24, 2022

zenmonkeykstop commented Mar 31, 2022

legoktm commented Mar 31, 2022

legoktm commented Mar 31, 2022

zenmonkeykstop left a comment

Choose a reason for hiding this comment

legoktm commented Mar 18, 2022 •

edited

codecov-commenter commented Mar 21, 2022 •

edited