Add feature test for JS-enabled document capture async upload#4407
Merged
Add feature test for JS-enabled document capture async upload#4407
Conversation
zachmargolis
approved these changes
Nov 13, 2020
Contributor
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM! thanks for adding this!
aduth
force-pushed
the
aduth-document-capture-async-feature-test
branch
from
5e8e6cc to
ddc7e0f
Compare
**Why**: While yet-unused in the implementation itself, it's part of the public API, so don't name as if it were an unused argujment. They'll become used in the future.
Contributor
Author
|
I can't find the comment here, but I received a notification about removing the underscore prefixes from the |
**Why**: Avoid lingering effects after test case has finished
aduth
force-pushed
the
aduth-document-capture-async-feature-test
branch
from
5bae43b to
4fea752
Compare
**Why**: Used to test in-progress result
Contributor
Author
|
I was finally able to work out what had caused the failures in the build environment: Because we were overriding |
aduth
added a commit
that referenced
this pull request
Nov 20, 2020
**Why**: So that the browser will allow connect to upload images to S3. The work in #4409 assumes that a wildcard `*` can be used in the path fragment of a `connect-src` source list, which is not correct ([see specification](https://www.w3.org/TR/CSP3/#match-paths)). Thus, connection attempts would still be rejected. This largely reverts the prior effort, instead appending to the CSP header in the view itself. This may not be ideal, though it has a few upsides: - Closer association to where the CSP URLs are relevant (as opposed to application-wide configuration) - [There is prior art](https://github.com/18F/identity-idp/blob/86981809d5e2d4a19e31b2ad89149b6737674402/app/views/shared/_recaptcha.html.erb#L2-L3) Alternatives explored: - Append at [point in code](https://github.com/18F/identity-idp/blob/86981809d5e2d4a19e31b2ad89149b6737674402/app/services/idv/steps/document_capture_step.rb#L18-L33) where URLs are assigned into view variables. This felt like an unexpected side-effect of what should be expected to be a pure function. - Some other pre-render hook of the document capture step. Unfortunately most existing behavior of a step appears to be tailored for a step's submission, not its initial rendering. Possible follow-on: Ideally the feature tests added in #4407 could exercise these headers. Unfortunately, it's made challenging by the fact that our fake endpoints are served by the local server, which is allowed by default in the CSP headers. This would also be contingent upon LG-3785.
aduth
added a commit
that referenced
this pull request
Nov 20, 2020
**Why**: So that the browser will allow connect to upload images to S3. The work in #4409 assumes that a wildcard `*` can be used in the path fragment of a `connect-src` source list, which is not correct ([see specification](https://www.w3.org/TR/CSP3/#match-paths)). Thus, connection attempts would still be rejected. This largely reverts the prior effort, instead appending to the CSP header in the view itself. This may not be ideal, though it has a few upsides: - Closer association to where the CSP URLs are relevant (as opposed to application-wide configuration) - [There is prior art](https://github.com/18F/identity-idp/blob/86981809d5e2d4a19e31b2ad89149b6737674402/app/views/shared/_recaptcha.html.erb#L2-L3) Alternatives explored: - Append at [point in code](https://github.com/18F/identity-idp/blob/86981809d5e2d4a19e31b2ad89149b6737674402/app/services/idv/steps/document_capture_step.rb#L18-L33) where URLs are assigned into view variables. This felt like an unexpected side-effect of what should be expected to be a pure function. - Some other pre-render hook of the document capture step. Unfortunately most existing behavior of a step appears to be tailored for a step's submission, not its initial rendering. Possible follow-on: Ideally the feature tests added in #4407 could exercise these headers. Unfortunately, it's made challenging by the fact that our fake endpoints are served by the local server, which is allowed by default in the CSP headers. This would also be contingent upon LG-3785.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #4401 (comment)
Why: So we have some integration tests running through document capture in a real headless browser.
Implementation Notes:
The biggest hurdle here was overcoming "Failed to fetch" errors in the browser, related to the fact that without the changes here, the presigned URLs were generated with a host of
example.com, whichwindow.fetchwould not connect with, since the test runs on localhost with a randomly-generated port. There may have been many alternative approaches here (e.g. avoidingexample.comand instead setting a fixed host/port for tests), but in this case I addressed it by overriding the default URL options for the URL helper in tests using the JavaScript test driver.