Fix areas the Journalist Interface does not gracefully degrade when Security Slider is High #1476
Comments
psivesely
changed the title
|
X-link #1544. It's up for debate whether we think that lack of "gracefully degrading" is a bug, but if we decide it's not we can't say we really support a Security Slider High compatible workflow on the JI, which is sad, and I believe we should. |
psivesely
pushed a commit
that referenced
this issue
Mar 14, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
psivesely
pushed a commit
that referenced
this issue
Mar 14, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
|
While I think this is worth doing, I don't think it's high priority enough to justify putting into 0.4. can we move this off the 0.4 release? |
psivesely
pushed a commit
that referenced
this issue
Mar 22, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
psivesely
pushed a commit
that referenced
this issue
Apr 6, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
psivesely
pushed a commit
that referenced
this issue
Apr 6, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
|
FYI I just noticed while making a user account on the admin interface that the Google authenticator QR code does not render when the Tor Browser security slider is set to high:
|
|
Great catch, @redshiftzero. Thankfully we display the TOTP seed value, but we'll still need to fix this. In general I do not expect that Journalists are setting the security slider to high, but I've still been testing locally with it set to high in order to catch issues like this one. |
|
@redshiftzero Can you make a separate issue for that? Updated my original comment to track it here in this "parent issue," but it would be good to split some of the tasks herein into individual "child issues." |
|
Let's just keep them listed in the checklist in this issue so we don't further exacerbate the rather tangled nest of issues that we have 🙃 |
redshiftzero
pushed a commit
that referenced
this issue
Apr 20, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
redshiftzero
pushed a commit
that referenced
this issue
Apr 20, 2017
* If a user tries to change a password for an account and enters the existing password, the operation is now idempotent. Similarly, if an admin "changes" a user's username to itself, this operation is also idempotent. The new `commit_account_changes` function only writes to the database if the `db.User` object of the user's account being changed has been locally modified, which it determines via the `Session.is_modified(db.User)` method. * The `/admin/edit/<int:user_id>` and `/account` routes now use the same template. Since the former provides a superset of the functionality of the latter, and there is no security risk in doing so, DRYing out the templates here made sense. While this fixes #1597, combining the templates also has potential to expose elements meant for admins to users in some future regression. To defend against this, I introduced a new functional test module, `tests.functional.make_account_changes`, which provides comprehensive checks to this regard. I also did some DRYing of these route functions since they both change passwords, commit account changes to the database, and flash messages regarding the success or failure of these operations. * I also made some improvements to the relevant flashed messages for account changes, making progress towards #1476, #1601, #1602. It seemed like "success" and "failure" were better categorical indicators than "notification" and "failure," so I went with that and used the green checkmark PNG we have already employed in the source interface. I changed all the flashed icons here to PNGs while I was at it to make a small step towards making the JI security-slider-friendly, and explicitly specified their sizes to make sure they rendered correctly. * Changing some of the flashed messages in the process of my refactor, I had to change the corresponding unit tests as well. To test both the content and category of the flashed message, I decided to take advantage of Flask-Testing's `assertMessageFlashed` method. This required installing an additional testing dependency, `blinker`, which I did without changing any other package versions.
ghost
added
redshiftzero
changed the title
|
All the named issues have since been addressed, except for the QR code issue, which is tracked in #3293. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
From some old issues made before I started here, we used to encourage the idea that some JS was acceptable as long as the interface gracefully degraded when it was disabled. There are some areas now where this doesn't quite seem to be the case.
JS is the
#1tool for anyone wishing to exploit a browser. Security by compartmentalization is a fundamental component of the SD security model. We're kind of forcing users to compromise the strength of that compartmentalization when we make the interface inaccessible w/o JS, even though it's fair to say most would not disable JS, even if the JI did degrade gracefully, simply because they wouldn't consider it. We should still consider the users who are disabling JS on their JWs, but better we should fix this for all users long-term via the reading room.I'm (re-)creating this issue to (i) create a list of some functionality that doesn't transition nicely between JS, and (ii) try to incorporate some practices for testing into the developer docs that will help contributors prevent the list in (i) from building up again.
The list (to be updated continually):
The text was updated successfully, but these errors were encountered: