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.

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

Prevent user from sending replies when client is not authenticated #256

Merged
merged 6 commits into from
Mar 11, 2019

Conversation

heartsucker
Copy link
Contributor

@heartsucker heartsucker commented Mar 6, 2019

Fixes #243

Testing:

  • Start client
  • Skip login
  • See warning instead of reply box
  • Sign in
  • See reply box
  • Sign out
  • See warning again

Also:

  • Check that the type annotations I added are correct

@eloquence eloquence added this to Ready for review in SecureDrop Team Board Mar 7, 2019
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

lgtm w/ one nitpick/ allie's-humble-opinion

  • Behavior works as documented in the PR description
  • Type annotations are helpful and look correct
  • New is_authenticated Client property lgtm, new authentication_state pyqt signal lgtm

[nitpick] Does it make sense to carry the concept of authentication all the way down to the widget level? E.g. This might read more clearly:
_on_authentication_update -> _show_replybox
self.controller.authentication_state.connect(self._on_authentication_update) -> self.controller.authentication_state.connect(self._show_replybox)
To me it's just a little easier to understand what the authentication state affects.

@sssoleileraaa sssoleileraaa moved this from Ready for review to Under Review in SecureDrop Team Board Mar 7, 2019
@heartsucker heartsucker merged commit 881301b into master Mar 11, 2019
SecureDrop Team Board automation moved this from Under Review to Done Mar 11, 2019
@heartsucker heartsucker deleted the block-replies branch March 11, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants