Move CryptoUtil.display_id() out of CryptoUtil to fix type-checking #6087
Conversation
nabla-c0d3
force-pushed
the
refactor-display-id
branch
2 times, most recently
from
5afe5fc to
1531a59
Compare
| @@ -116,7 +116,7 @@ def set_source_count(s: str) -> int: | |||
|
|
|||
|
|
|||
| def add_journalist( | |||
| username: str = "", | |||
| username: str, | |||
There was a problem hiding this comment.
username is now a required argument to simplify the logic here (and improve typing).
| @@ -84,8 +71,6 @@ class CryptoUtil: | |||
|
|
|||
| def __init__(self, | |||
| securedrop_root: str, | |||
| nouns_file: str, | |||
| adjectives_file: str, | |||
There was a problem hiding this comment.
This was moved to the new _DesignationGenerator class.
| new_designation = ' '.join([random.choice(self.adjectives), | ||
| random.choice(self.nouns)]) | ||
|
|
||
| collisions = Source.query.filter(Source.journalist_designation == new_designation) |
There was a problem hiding this comment.
This was a bit problematic because it pulls a database session "out of thin air" (by doing Source.query()) instead of database_session.query()), so display_id() could only work when called from the correct context, which can get complicated (for example in unit tests).
| except ValueError: | ||
| # Create a unique journalist designation for the source | ||
| # TODO: Add unique=True to models.Source.journalist_designation to enforce uniqueness | ||
| # as the logic below has a race condition (time we check VS time when we add to the DB) |
There was a problem hiding this comment.
I left a TODO here to not have a to do a DB migration. The race condition already existed in the previous code base. However, it may also not be a "real" problem.
| def test_encrypt_then_decrypt_gives_same_result( | ||
| source_app, | ||
| test_source, | ||
| name, | ||
| secret, |
There was a problem hiding this comment.
Not used by the test.
nabla-c0d3
force-pushed
the
refactor-display-id
branch
from
1531a59 to
caeaf1a
Compare
nabla-c0d3
force-pushed
the
refactor-display-id
branch
from
caeaf1a to
ab25f8f
Compare
nabla-c0d3
force-pushed
the
refactor-display-id
branch
from
ab25f8f to
895fe8b
Compare
nabla-c0d3
changed the title
Status
Ready.
Description of Changes
This small PR refactors
CryptoUtil.display_id()and moves it out ofCryptoUtilin order to:display_id()for CryptoUtil code is not type-checked due to dynamic lookup/usage #5599 .I tested this in the dev environment and checked that journalist designations are properly generated.