Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

3138 revoke tokens on logout #3149

Merged
merged 3 commits into from
Mar 24, 2023
Merged

3138 revoke tokens on logout #3149

merged 3 commits into from
Mar 24, 2023

Conversation

VakarisZ
Copy link
Contributor

What does this PR do?

Fixes #3138

Add any further explanations here.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Do all unit tests pass?
  • Do all end-to-end tests pass?
  • Any other testing performed?

    Tested by
    logout_revoke

@VakarisZ VakarisZ requested a review from mssalvatore March 24, 2023 13:50
VakarisZ
VakarisZ commented Mar 24, 2023
View reviewed changes
monkey/monkey_island/cc/services/authentication_service/authentication_facade.py
):
self._repository_encryptor = repository_encryptor
self._island_event_queue = island_event_queue
self._datastore = user_datastore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use datastore than directly manipulating the database. It will be easier to change if we migrate to another db + less overriding required

@codecov
Copy link

codecov bot commented Mar 24, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.03 ⚠️

Comparison is base (83557c9) 71.66% compared to head (320bc6e) 71.64%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #3149      +/-   ##
===========================================
- Coverage    71.66%   71.64%   -0.03%     
===========================================
  Files          452      453       +1     
  Lines        12852    12864      +12     
===========================================
+ Hits          9210     9216       +6     
- Misses        3642     3648       +6

see 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

cakekoa
cakekoa reviewed Mar 24, 2023
View reviewed changes
monkey/monkey_island/cc/services/authentication_service/authentication_facade.py Outdated
@@ -1,3 +1,5 @@
from flask_security import MongoEngineUserDatastore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything preventing us from using UserDatastore?

@VakarisZ VakarisZ force-pushed the 3138-revoke-tokens-on-logout branch from 4c69d1f to 5700f1d Compare March 24, 2023 14:50
mssalvatore
mssalvatore approved these changes Mar 24, 2023
View reviewed changes
Copy link
Collaborator

@mssalvatore mssalvatore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, as long as manual testing has been done to ensure authentication is still enforced.

cakekoa
cakekoa reviewed Mar 24, 2023
View reviewed changes
...s/unit_tests/monkey_island/cc/services/authentication_service/test_authentication_service.py Outdated
@@ -30,13 +31,20 @@ def mock_island_event_queue(autouse=True) -> IIslandEventQueue:
return MagicMock(spec=IIslandEventQueue)


@pytest.fixture
def mock_user_datastore(autouse=True) -> MongoEngineUserDatastore:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use UserDatastore here, since that's what AuthenticationFacade expects

@VakarisZ VakarisZ force-pushed the 3138-revoke-tokens-on-logout branch from 402e15d to 320bc6e Compare March 24, 2023 15:35
@mssalvatore mssalvatore force-pushed the 3138-revoke-tokens-on-logout branch from 320bc6e to 5464788 Compare March 24, 2023 15:47
@mssalvatore mssalvatore merged commit 59fed74 into develop Mar 24, 2023
@mssalvatore mssalvatore deleted the 3138-revoke-tokens-on-logout branch March 24, 2023 15:48
@An0nYm0u5101 An0nYm0u5101 mentioned this pull request Nov 28, 2023
Open
@muyenzo muyenzo mentioned this pull request Nov 30, 2023
Open
@An0nYm0u5101 An0nYm0u5101 mentioned this pull request Dec 1, 2023
Open
@muyenzo muyenzo mentioned this pull request Dec 19, 2023
Open
@An0nYm0u5101 An0nYm0u5101 mentioned this pull request Dec 21, 2023
Open
@muyenzo muyenzo mentioned this pull request May 20, 2024
Open
@An0nYm0u5101 An0nYm0u5101 mentioned this pull request May 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cakekoa cakekoa cakekoa approved these changes

@mssalvatore mssalvatore mssalvatore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revoke tokens on logout
3 participants
@VakarisZ @cakekoa @mssalvatore