Account Delete: Require Password (LG-2964) by zachmargolis · Pull Request #3775 · 18F/identity-idp

zachmargolis · 2020-05-18T18:15:31Z

Still waiting on confirmation for designs and copy, making this a draft for now, also to see if there are any spec failures in the rest of the acceptance tests

Before

After

**Why**: To prevent drive-by deletions from leaving a screen unlocked

**Why**: They're still used

…e-password

jmhooper · 2020-05-18T19:57:52Z

app/controllers/users/delete_controller.rb

@@ -1,6 +1,7 @@
 module Users
-  class DeleteController < ReauthnRequiredController
+  class DeleteController < ApplicationController


I noticed reviewing this that this controller doesn't track any analytics events. How'd you feel about adding those here?

Happy to add them! Good idea, thanks

jmhooper · 2020-05-18T19:58:43Z

app/views/users/delete/show.html.erb

@@ -0,0 +1,30 @@
+<div class="p0 cntnr-xxskinny border-box bg-white rounded-xxl modal-warning">


Another slim file bites the dust 💯

jmhooper · 2020-05-18T20:00:41Z

Oops, just noticed this was a draft. Looks like you're still working on the tests

zachmargolis · 2020-05-18T20:42:44Z

spec/features/two_factor_authentication/change_factor_spec.rb

      end
    end
-
-    scenario 'deleting account' do


cc @jmhooper this spec checked that we were prompted for 2fa before deleting account, basically testing that the controller inherited from the Reauth controller. I figured that by prompting for the password we'd be OK without, but I think that controller also checks another factor. Do you think it's still OK to remove this?

Yep, I think this is obsolete now

zachmargolis · 2020-05-18T20:43:06Z

This is ready for review now (pending design changes)

jmhooper

Still lgtm

* Update saml_idp revision * Lg 2939 update us gov banner (#3751) Add 'Here's how you know' section LG-2939 **Why**: So users can be confident in legitimacy/security of the site **How**: Base layout renders single ERB banner; JS enables toggling Co-authored-by: Nick Ng <nick.ng@gsa.gov> * [Snyk] Fix for 1 vulnerabilities (#3746) * fix: package.json & .snyk to reduce vulnerabilities The following vulnerabilities are fixed with a Snyk patch: - https://snyk.io/vuln/SNYK-JS-LODASH-567746 * Copy over .snyk too Co-authored-by: Zach Margolis <zachary.margolis@gsa.gov> * Add spec to make sure that re-proofing costs behave as expected (#3754) * Fix spec to reflect the fact that we've removed SingleLogoutService * Revert "[Snyk] Fix for 1 vulnerabilities (#3746)" (#3755) This reverts commit 9323c2a. * fix bundler config to work (#3759) * Add deal and IAA information to service_providers table (LG-2865) (#3756) * Add deal and IAA information to service_providers table (LG-2865) **Why**: So that we can track metadata associated with SPs better * Various accessibility Fixes (#3761) * Fix TOTP setup nickname label (LG-2986) **Why**: Screen readers use this to describe the field, the old label was for a different field * Update accordion header's role to be a button (LG-2985) **Why**: It's intended to be clicked to show/hide information * Make the TOTP key copy button a button (LG-2987) **Why**: So it can be detected by screen readers as clickable * Make cancel link a button role on account deletion page (LG-2988) **Why**: Better screen reader support * Make sure hidden webauthn element is skipped by screen readers (LG-2984) (#3762) **Why**: Merely styling it as hidden, or setting aria-hidden does not hide sufficiently from screen readers, so we use the HTML5 "hidden" attribute to do this. * Update GSA logo alt text for screen readers (LG-2983) (#3763) **Why**: The alt text and link text both described GSA so now it's just the link text * LG-2959 Add a missing `return` statement when a Acuant SDK file is not permitted (#3765) **Why**: So the execution does not continue leading to a 500 error * Remove load testing scripts (#3764) **Why**: They were moved to their own repository (https://github.com/18f/identity-loadtest/) * Fix typo in method name (#3766) * Update order of elements in footer to match visual order (LG-2989) (#3768) * fix: package.json to reduce vulnerabilities (#3757) The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-YARGSPARSER-560381 * Remove the Acuant SDK from the desktop flow (#3767) **Why**: People using the desktop flow have to use a file picker already. Having the SDK there only means they have to wait for it to load. * LG-2375 Don't truncate the return to SP link (#3770) **Why**: SPs have names of varying length. Before this commit you can't read the name of SPs with long names. * Fix positioning of remember-this-browser checkbox (#3769) * Fix positioning of remember-this-browser checkbox **Why**: LG-1047 - chk + label are on diff. lines on some mobile devices **How**: Added flex container and leveraged several USWDS classes * Cleanup **Why**: To improve ERB template readability **How**: Better indentation; hash keys on multiple lines * Update yarn.lock (#3771) - To match package.json updates in #3757 * LG-2936 Add liveness check to main doc auth flow (#3758) * Hide/show email error messages via HTML5 hidden attribute (LG-2996) (#3773) **Why**: We got reports from an accessibility audit that IE 11 + JAWS was still reading these even though they're hidden by CSS * LG-260 Separate otp request tracking and throttling for verified phones (#2388) **Why**: So throttling a user with an unverified phone does not throttle a user with a verified phone and we can track these otp requests separately. **How**: Add a new phone_confirmed true/false column to the otp_requests_trackers. Add a new unique index for phone_fingerprint + phone_confirmed. Then drop the unique index for phone_fingerprint. Force any interaction with the table to use the new composite key. * Bring back i18n keys removed in #3767 (#3776) **Why**: They're still used * Account Delete: Require Password (LG-2964) (#3775) * Update account delete to require a password on the page (LG-2964) **Why**: To prevent drive-by deletions from leaving a screen unlocked * users/delete#show: slim to ERB conversion * Add analytics tracking Co-authored-by: Shade L. Jenifer <shade.jenifer@gsa.gov> Co-authored-by: Nick Ng <nick.ng@gsa.gov> Co-authored-by: Snyk bot <snyk-bot@snyk.io> Co-authored-by: Zach Margolis <zachary.margolis@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Tim Spencer <timothy.spencer@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Steve Urciuoli <steve.urciuoli@gsa.gov>

zachmargolis added 4 commits May 18, 2020 10:27

users/delete#show: slim to ERB conversion

92bd8c9

Update account delete to require a password on the page (LG-2964)

2c4f378

**Why**: To prevent drive-by deletions from leaving a screen unlocked

Bring back i18n keys removed in #3767

c9644e9

**Why**: They're still used

Merge branch 'margolis-recover-i18n-keys' into margolis-account-delet…

6c694a2

…e-password

jmhooper approved these changes May 18, 2020

View reviewed changes

zachmargolis added 4 commits May 18, 2020 13:18

rubocop

fdce6de

Fix a few acceptance specs

8ca0f82

Add analytics tracking

889af3b

Remove spec tied to reauthn controller

7aebffc

zachmargolis marked this pull request as ready for review May 18, 2020 20:40

zachmargolis commented May 18, 2020

View reviewed changes

zachmargolis requested review from achapm, slj and stevegsa May 18, 2020 20:42

zachmargolis added the status - ready for review label May 18, 2020

jmhooper approved these changes May 18, 2020

View reviewed changes

Visual update: make dots normal color, not yellow

e5c9ecd

zachmargolis merged commit 309042c into master May 19, 2020

zachmargolis deleted the margolis-account-delete-password branch May 19, 2020 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account Delete: Require Password (LG-2964)#3775

Account Delete: Require Password (LG-2964)#3775
zachmargolis merged 9 commits intomasterfrom
margolis-account-delete-password

zachmargolis commented May 18, 2020

Uh oh!

jmhooper May 18, 2020

Uh oh!

zachmargolis May 18, 2020

Uh oh!

jmhooper May 18, 2020

Uh oh!

jmhooper commented May 18, 2020

Uh oh!

zachmargolis May 18, 2020

Uh oh!

jmhooper May 18, 2020

Uh oh!

zachmargolis commented May 18, 2020

Uh oh!

jmhooper left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,30 @@
		<div class="p0 cntnr-xxskinny border-box bg-white rounded-xxl modal-warning">

Conversation

zachmargolis commented May 18, 2020

Uh oh!

jmhooper May 18, 2020

Choose a reason for hiding this comment

Uh oh!

zachmargolis May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jmhooper May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jmhooper commented May 18, 2020

Uh oh!

zachmargolis May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jmhooper May 18, 2020

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented May 18, 2020

Uh oh!

jmhooper left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants