Add a route for continuing user authorization confirmation#5735
Add a route for continuing user authorization confirmation#5735
Conversation
**Why**: Previously this action was handled by the completions controller. This added some not-obvious coupling between the two. As a result, changes to the completions controller update action can have unintended consequences. This commit separates them out to hopefull simplify things. There is some logic in the completions controller to support both actions that we can also remove, but we will need to wait until this code is fully deployed before we pull that out.
|
The |
|
As an added bonus, we now get analytics events for these actions that are confusingly associated with the completions controller |
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Maybe a silly question, but: How does one test this flow?
| AUTHENTICATION_CONFIRMATION_CONTINUE = 'Authentication Confirmation: Continue selected'.freeze | ||
| AUTHENTICATION_CONFIRMATION_RESET = 'Authentication Confirmation: Reset selected'.freeze |
There was a problem hiding this comment.
Can we check that these events are logged somewhere in specs (e.g. feature specs for this flow)?
There was a problem hiding this comment.
Ah yeah, I wrote a controller spec with the idea to test these and never actually tested that they were logged
There was a problem hiding this comment.
Just pushed some checks for these events
There was a problem hiding this comment.
The other thing I was curious about is if we were logging anything previously that we care to be continuing to log in this new redirect flow.
Go to one sample SP, and sign in to your login.gov account. Visit a different sample SP, and sign in. You should be confronted with this screen:
Both actions on this screen are now backed by the |
|
Also, for reference, the feature tests for this flow are here for oidc and here for saml. This change does not impact how the feature works so it did not necessitate and changes to these tests. |
| AUTHENTICATION_CONFIRMATION_CONTINUE = 'Authentication Confirmation: Continue selected'.freeze | ||
| AUTHENTICATION_CONFIRMATION_RESET = 'Authentication Confirmation: Reset selected'.freeze |
There was a problem hiding this comment.
The other thing I was curious about is if we were logging anything previously that we care to be continuing to log in this new redirect flow.
One of the things that inspired this change was looking at the completions controller and realizing that there was not logging on these events. The completions controller actually had a condition to not log in the case addressed here. |
This commit removes some unnecessary logic: - Removes ialmax method which was unused - Removes logic around whether the completions screen is required since now it is required in all contexts (ref: #5735)
* Add a route for continuing user authorization confirmation **Why**: Previously this action was handled by the completions controller. This added some not-obvious coupling between the two. As a result, changes to the completions controller update action can have unintended consequences. This commit separates them out to hopefully simplify things. There is some logic in the completions controller to support both actions that we can also remove, but we will need to wait until this code is fully deployed before we pull that out.
This commit removes some unnecessary logic: - Removes ialmax method which was unused - Removes logic around whether the completions screen is required since now it is required in all contexts (ref: #5735)
This commit removes some unnecessary logic: - Removes ialmax method which was unused - Removes logic around whether the completions screen is required since now it is required in all contexts (ref: #5735)
2 participants
Why: Previously this action was handled by the completions controller. This added some not-obvious coupling between the two. As a result, changes to the completions controller update action can have unintended consequences. This commit separates them out to hopefull simplify things.
There is some logic in the completions controller to support both actions that we can also remove, but we will need to wait until this code is fully deployed before we pull that out.