Delay mongo init to after registration#1495
Conversation
shreyamalviya requested review from VakarisZ, ilija-lazoroski and mssalvatore 
Codecov Report
@@ Coverage Diff @@ ## develop #1495 +/- ## =========================================== - Coverage 42.61% 42.09% -0.53% =========================================== Files 476 468 -8 Lines 14193 14069 -124 =========================================== - Hits 6049 5922 -127 - Misses 8144 8147 +3
Continue to review full report at Codecov.
|
| | ||
| if _credentials_match_registered_user(username, password): | ||
| access_token = _create_access_token(username) | ||
| _check_attack_mitigations_in_mongo() |
There was a problem hiding this comment.
This checks for attack mitigations during login? Why? Shouldn't it happen during registration?
| def _check_attack_mitigations_in_mongo(): | ||
| if AttackMitigations.COLLECTION_NAME not in mongo.db.list_collection_names(): | ||
| init_collections() |
There was a problem hiding this comment.
I feel like this function doesn't belong here since it has nothing to do with authentication's resource
| is_registration_needed = env_singleton.env.needs_registration() | ||
| if is_registration_needed: | ||
| # if registration is required, drop previous user's data (for credentials reset case) | ||
| _drop_mongo_db() |
There was a problem hiding this comment.
All this logic should be moved into init_collections method which gets called during registration. This way interaction with mongodb during registration is separated from the logic of resources. In general resources shouldn't interact with database directly.
| except Exception as ex: | ||
| logger.error( | ||
| "Exception raised during registration; most likely an issue with the " | ||
| f"mongo collection's initialisation. Exception: {str(ex)}." | ||
| ) |
There was a problem hiding this comment.
if the error has to do with interaction with mongo we should catch it when interacting with mongo.
| "Exception raised during registration; most likely an issue with the " | ||
| f"mongo collection's initialisation. Exception: {str(ex)}." | ||
| ) | ||
| return make_response({"error": str(ex)}, 400) |
There was a problem hiding this comment.
What's the poin of this response? user can already see the error in logs and we don't handle 400 in any special way on the front end.
| | ||
| | ||
| def _drop_mongo_db(): | ||
| mongo.db.command("dropDatabase") |
There was a problem hiding this comment.
why not mongo.drop_database('db')? Why are we even dropping the whole database if we're going to import the exact same mitigations?
VakarisZ left a comment
VakarisZ left a comment There was a problem hiding this comment.
Mostly nit-picking on the readability, but some major questions as well.
CHANGELOG.md Outdated
| - Initialise MongoDB collection of attack mitigations after registration or login (if required) | ||
| instead of on Island startup. #1495 |
There was a problem hiding this comment.
I think this is too low-level for the changelog.
Maybe a better entry is "Resetting credentials clears the database".
…esetting login credentials also cleans the contents of the database. #1495"
… in DB, add if not
of when registration request is sent The issue with this whole change is that there's a long gap where nothing happens after you click on the log in or register button on the UI. But we don't need to worry about this because we plan on shipping Island's mongodb with attack mitigations already present.
…esetting login credentials also cleans the contents of the database. #1495"
…st is being processed
VakarisZ force-pushed the delay-mongo-init branch from 8e425b1 to 579ebf4 Compare | def reset_database(): | ||
| Database.reset_db() | ||
| if Database.is_mitigations_missing(): | ||
| logger.info("Populating Monkey Island with ATT&CK mitigations, this might take a while...") |
There was a problem hiding this comment.
This happens very quickly now (< 5ms). I think we can remove "this might take a while..."
…he part saying that it will take a while
4 participants
What does this PR do?
#1463
PR Checklist
Testing Checklist
Added relevant unit tests?