LG 363 LOA3 cancellation by jmhooper · Pull Request #2341 · 18F/identity-idp

jmhooper · 2018-07-18T21:35:21Z

Why: We've re-designed the cancellation flow for LOA3 to make it a bit more straightforward. This means adding a new page for cancellations and removing the personal key confirmation (assuming the user will care more about when they come back later and reset their account if they don't have it).

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

For DB changes, check for missing indexes, check to see if the changes
affect other apps (such as the dashboard), make sure the DB columns in the
various environments are properly populated, coordinate with devops, plan
migrations in separate steps.
For route changes, make sure GET requests don't change state or result in
destructive behavior. GET requests should only result in information being
read, not written.
For encryption changes, make sure it is compatible with data that was
encrypted with the old code.
For secrets changes, make sure to update the S3 secrets bucket with the
new configs in all environments.
Do not disable Rubocop or Reek offenses unless you are absolutely sure
they are false positives. If you're not sure how to fix the offense, please
ask a teammate.
When reading data, write tests for nil values, empty strings,
and invalid formats.
When calling redirect_to in a controller, use _url, not _path.
When adding user data to the session, use the user_session helper
instead of the session helper so the data does not persist beyond the user's
session.
When adding a new controller that requires the user to be fully
authenticated, make sure to add before_action :confirm_two_factor_authenticated.

jmhooper · 2018-07-18T21:36:01Z

app/assets/stylesheets/components/all.scss

 @import 'personal-key';
 @import 'position';
 @import 'tooltip';
-@import 'icon';


@mryenq: do you remember why we deleted this?

@jmhooper Currently it's duplicated:

identity-idp/app/assets/stylesheets/components/all.scss

Line 11 in 0327095

@import 'icon';

identity-idp/app/assets/stylesheets/components/all.scss

Line 23 in 0327095

@import 'icon';

jmhooper · 2018-07-18T21:37:25Z

config/routes.rb

        get '/jurisdiction' => 'jurisdiction#new'
        post '/jurisdiction' => 'jurisdiction#create'
        get '/jurisdiction/failure/:reason' => 'jurisdiction#failure', as: :jurisdiction_failure
+        get '/cancel/:step' => 'cancellations#new', as: :cancel_step


This is one of the things I'm unsure about. We are putting the step in the path so we can know which step to route the user back to. The other option is to put the go back step in the session. I'm 50/50 on which is better so hoping to get some thoughts from reviewers.

Per our Slack conversation, can't we just use <%= link_to 'Back', :back %>? It looks like the Referer header is available in the request and it will only try to use javascript IF the Referer is missing.

The Referer header won't be present if a user goes directly to the cancel page (or refreshes it) so it won't happen normally, but its definitely possible. So in the case where: Javascript is not enabled AND the user refreshes or navigates to the cancel page via the url, the link could send them to /verify since this is supposed to automatically take them to the correct step, but currently doesn't handle the jursidiction step correctly.

We could also back the entire 'back' url as a query parameter, but we'll lose the 'friendly url' goal.

I'm always uncomfortable putting information in the user session that is tied to the page they're on. In general, it's bad practice because it prevents people from having multiple tabs open on the same site with the same login session (e.g., consider how something like Google Docs might behave if its state was stored in the user session and not scoped to the document). It can also break the back button.

One way I've done this before is to store state in a secure fashion in the page as a hidden variable. This ties it to where they are in the process (the page) so the back button is not broken. It lets them do multiple things at once on the site (unlikely for login.gov). It makes the state information tamper-proof (encryption). Not relying on the URL to tell us their state prevents someone from jumping into the middle of a process if we want to control the flow through that process.

Generally, the user session should hold data that applies to all pages they access regardless of what they are trying to do (it's global data). Contextual or local data should be managed outside of the global data store/user session.

konklone · 2018-07-18T23:39:04Z

Could you describe the changes more thoroughly, or link to a ticket or doc where they are?

davemcorwin · 2018-07-19T00:14:03Z

config/routes.rb

        get '/jurisdiction' => 'jurisdiction#new'
        post '/jurisdiction' => 'jurisdiction#create'
        get '/jurisdiction/failure/:reason' => 'jurisdiction#failure', as: :jurisdiction_failure
+        get '/cancel/:step' => 'cancellations#new', as: :cancel_step


Per our Slack conversation, can't we just use <%= link_to 'Back', :back %>? It looks like the Referer header is available in the request and it will only try to use javascript IF the Referer is missing.

The Referer header won't be present if a user goes directly to the cancel page (or refreshes it) so it won't happen normally, but its definitely possible. So in the case where: Javascript is not enabled AND the user refreshes or navigates to the cancel page via the url, the link could send them to /verify since this is supposed to automatically take them to the correct step, but currently doesn't handle the jursidiction step correctly.

davemcorwin · 2018-07-19T00:16:53Z

config/routes.rb

        get '/jurisdiction' => 'jurisdiction#new'
        post '/jurisdiction' => 'jurisdiction#create'
        get '/jurisdiction/failure/:reason' => 'jurisdiction#failure', as: :jurisdiction_failure
+        get '/cancel/:step' => 'cancellations#new', as: :cancel_step


We could also back the entire 'back' url as a query parameter, but we'll lose the 'friendly url' goal.

davemcorwin · 2018-07-19T00:27:39Z

app/views/idv/cancellations/destroy.html.slim

+  - @presenter.cancellation_effects.each do |effect|
+    li = effect
+
+.mt2 = link_to t('idv.cancel.return_to_account'), account_path


Ideally we want to send them back to the SP here. The idea is that each SP will configure a 'failure to verify' url (I think RRB has one but its not configured). Until thats added, we should at least send them to the return_to_sp_url (sp_return_url on the decorated session) The generated url may include the access_denied status which we may have to document what the SP can expect. Alternatively, we can use the learn_more links that are hardcoded in the decorated session.

Yeah, lemme try the link_to :back and see what I can figure out.

Also, on the link to the sp return url, we decided to handle that in a separate follow-on PR since this one is already pretty hefty. There's a separate ticket for it this sprint.

jgsmith-usds · 2018-07-19T14:06:38Z

app/assets/stylesheets/components/_list.scss

-      vertical-align: -3px;
-    }
-  }
+  @include color-list();


@include color-list($yellow);?

@jgsmith-usds Taking advantage of default parameters for the mixin (but don't need to).

I'm a lot more comfortable specifying the color here since the goal is yellow dots. If the default ever changed, the color wouldn't match. Now, if we had .default-dots, then I'd probably want to not have an explicit color. :-)

jgsmith-usds · 2018-07-19T14:07:32Z

app/controllers/idv/cancellations_controller.rb

+    private
+
+    def current_step
+      params.require(:step).to_sym


I'm a bit uncomfortable allowing someone to jump into the middle of the steps. How do we know they should be on that step?

We know they should be on that step because they clicked the cancel link on that step, landing them here. Also, working now on getting rid of the current_step in favor of the referrer.

jgsmith-usds · 2018-07-19T14:24:11Z

config/locales/idv/en.yml

+        warning_1: We won't be able to verify your identity
+        warning_2: We won't keep a record of your name, address, birth date, or Social
+          Security number
+        warning_3: You won't be able to securely access you information using %{app}


you => your?

jmhooper · 2018-07-19T14:55:42Z

@konklone: The ticket is here: https://cm-jira.usa.gov/browse/LG-363

jmhooper · 2018-07-19T20:13:23Z

Okay, I've changed it up to use the referrer header for the back button. If the referrer header either doesn't exist or doesn't look right, it'll default to /verify which should (eventually) get the user back on track. That said, it shouldn't happen unless they visit the cancel path directly.

Also note that I did not use rails's link_to 'Back', :back method because that depends on JS in the absence of a referrer header which is not what I think we want.

I like the way the code came out with this method much better. PTAL!

jmhooper · 2018-07-19T20:14:24Z

app/controllers/idv/cancellations_controller.rb

+    before_action :confirm_idv_needed
+
+    def new
+      analytics.track_event(Analytics::IDV_CANCELLATION)


I realized this was not being tracked at all, so new analytics events just for you @monfresh

jmhooper · 2018-07-19T20:19:08Z

Actually, don't ptal yet. Looks like I messed up one of the views

jmhooper · 2018-07-19T20:28:35Z

Okay, we should be ready for another look now 👍

davemcorwin · 2018-07-19T21:27:32Z

app/presenters/idv/cancellation_presenter.rb

+
+    private
+
+    def referer_path


Looks reasonable but more complicated than I would have thought, makes me dream of Elixir-esque pattern matching...

Does the scheme matter if we're checking the host?

Also extract_path_and_query_from_uri COULD be

[uri.path, uri.query].compact.join('/')

not sure if thats more understandable to peeps other than me.

davemcorwin

LGTM

jmhooper and others added 10 commits July 11, 2018 15:46

wip

35cd11a

Add cancel links and tests

c8f42fe

Format cancellation templates, add translations and modify specs

d532d97

Unquoted inverted question mark no es bueno

48d6172

Add link to the session success screen

65aa51b

fix some specs

faaaa00

Add translations for cancel screen

b631670

Cleanup todo comments

d8aaa78

Merge branch 'master' into lg-363-loa3-cancel

288a8d1

cleanup and fix old specs

80c5614

jmhooper added the status - ready for review label Jul 18, 2018

jmhooper requested review from davemcorwin, jgsmith-usds, monfresh, mryenq and stevegsa July 18, 2018 21:35

jmhooper commented Jul 18, 2018

View reviewed changes

make the linters happy

c9ff9f2

davemcorwin reviewed Jul 19, 2018

View reviewed changes

jgsmith-usds reviewed Jul 19, 2018

View reviewed changes

jmhooper added status - work in progress and removed status - ready for review labels Jul 19, 2018

jmhooper added 4 commits July 19, 2018 10:55

remove the step from the path

bc86d27

typo

84b2b3d

scss fixup

ec518af

track analytics

3ee5a97

jmhooper added the status - ready for review label Jul 19, 2018

jmhooper removed the status - work in progress label Jul 19, 2018

jmhooper commented Jul 19, 2018

View reviewed changes

Dont allow JS urls

9d71b5d

jmhooper added status - work in progress and removed status - ready for review labels Jul 19, 2018

fix the view

6f61b86

jmhooper added status - ready for review and removed status - work in progress labels Jul 19, 2018

davemcorwin reviewed Jul 19, 2018

View reviewed changes

davemcorwin approved these changes Jul 19, 2018

View reviewed changes

jmhooper added 4 commits July 20, 2018 13:52

Merge branch 'master' into lg-363-loa3-cancel

314b6fb

clean up path/query join

5ad96b9

oops

d0c6673

array join method does not quite work

8b1fa40

jmhooper merged commit b67af2f into master Jul 20, 2018

snyk-bot mentioned this pull request Jun 1, 2020

[Snyk] Security upgrade @rails/webpacker from 4.0.7 to 4.1.0 o1898/identity-idp#11

Open

snyk-bot mentioned this pull request Aug 16, 2020

[Snyk] Security upgrade @rails/webpacker from 4.0.7 to 4.3.0 o1898/identity-idp#14

Open

amathews-fs deleted the lg-363-loa3-cancel branch January 7, 2021 18:39

aduth mentioned this pull request Feb 8, 2022

Remove unused IdV cancel view #5922

Merged

Conversation

jmhooper commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konklone commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhooper commented Jul 19, 2018

Uh oh!

jmhooper commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhooper commented Jul 19, 2018

Uh oh!

jmhooper commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davemcorwin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmhooper commented Jul 18, 2018 •

edited

Loading