Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
324af20
WIP image upload API work
ironman5366 Jul 20, 2020
ed40895
:wrench: WIP fixes and testing code
ironman5366 Jul 21, 2020
f9bf1c9
:wrench: WIP additions and changes to image upload API
ironman5366 Jul 22, 2020
1870b3a
WIP improvements on image API structure and tie ins
ironman5366 Jul 23, 2020
1e1ef4e
Merge branch 'master' into will-3204
ironman5366 Jul 23, 2020
a9a58e0
Merge branch 'master' into will-3204
ironman5366 Jul 23, 2020
ec7a44f
:wrench: WIP integrations with new image API
ironman5366 Jul 27, 2020
519a058
Start working on image specs and move liveness checking
ironman5366 Jul 28, 2020
0870a2d
Small fixes & corrections
ironman5366 Jul 28, 2020
f4abfbe
Linter fixes
ironman5366 Jul 28, 2020
7fd72f6
Retry tests
ironman5366 Jul 28, 2020
80b6467
Merge branches 'master' and 'will-3204' of https://github.com/18F/ide…
ironman5366 Jul 29, 2020
e5f3c9c
Store the response better in the session
ironman5366 Jul 29, 2020
190ba85
Spec & lint fixes
ironman5366 Jul 29, 2020
fc6654c
Small :bug: fix
ironman5366 Jul 29, 2020
2bb4deb
Merge branch 'master' into will-3204
aduth Aug 6, 2020
ad271a0
Make CSRF available as value of UploadContext
aduth Aug 5, 2020
d6fda5f
Wire upload to send form values payload
aduth Aug 6, 2020
b220a13
Merge branch 'master' into will-3204
jmhooper Aug 10, 2020
55e1fcc
Merge branch 'master' into will-3204
aduth Aug 12, 2020
f8d26ed
Refactor UploadContextProvider to remove propTypes
aduth Aug 12, 2020
a3acc63
Fix broken tests related to input key change
aduth Aug 12, 2020
de321a2
Add type detail to upload implementation
aduth Aug 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/controllers/concerns/idv_session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ def liveness_upgrade_required?
sp_session[:ial2_strict] && !current_user.active_profile&.includes_liveness_check?
end

def liveness_checking_enabled?
FeatureManagement.liveness_checking_enabled? && (no_sp? || sp_session[:ial2_strict])
end

def no_sp?
sp_session[:issuer].blank?
end

def confirm_idv_vendor_session_started
return if flash[:allow_confirmations_continue]
redirect_to idv_doc_auth_url unless idv_session.proofing_started?
Expand Down
73 changes: 73 additions & 0 deletions app/controllers/idv/image_upload_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
module Idv
class ImageUploadController < ApplicationController
include IdvSession

def upload
validation_error = validate_request(request)
response = validation_error || upload_and_check_images
render json: response
end

private

def upload_and_check_images
doc_response = client.post_images(front_image: @front_image,
back_image: @back_image,
selfie_image: @selfie_image,
liveness_checking_enabled: liveness_checking_enabled?)
return error_json(doc_response.errors.first) unless doc_response.success?
upload_info = {
documents: doc_response.to_h,
}
store_pii(doc_response)
user_session['idv/doc_auth']['api_upload'] = upload_info
success_json('Uploaded images')
end

def store_pii(doc_response)
user_session['idv/doc_auth'][:pii_from_doc] = doc_response.pii_from_doc.merge(
uuid: current_user.uuid,
phone: current_user.phone_configurations.take&.phone,
)
end

def validate_request(request)
steps = %i[check_content_type check_image_fields]
steps.each do |step|
err = method(step).call(request)
return error_json(err) if err
end
nil
Comment on lines +35 to +40
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With only two steps.... I don't think this is a great use for this level of dynamic behavior, there are a few alternatives I would prefer (in order of most to least preferred)

  1. Let's make a Form object -- we do this is many other controllers, make a form object that performs the validations and returns useful error structures the controllers know how to render (LMK if you need a more detailed example)

  2. Move these into before actions, so that way they can just render and stop the chain (instead of having a loop to check for errors and stop)

     before_action :check_content_type, only: [:upload]
     before_action :check_image_fields, only[:upload]
  3. Just call the methods directly:

    error = check_content_type(request) || check_image_fields(request)
    error_json(err) ir err

end

def check_content_type(request)
Copy link
Copy Markdown
Contributor

@aduth aduth Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked my original comment as resolved, but flagging @zachmargolis's comment at #3985 (comment) for follow-up.

Also should we 415 for this? https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/415

"Invalid content type #{request.content_type}" if request.content_type != 'application/json'
end

def check_image_fields(request)
data = request.body.read
@front_image = data['front']
@back_image = data['back']
@selfie_image = data['selfie']
'Missing image keys' unless [@front_image, @back_image, @selfie_image].all?
end
Comment on lines +47 to +53
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a side effect of setting the instance variables, so I think the name check_ is a little misleading.... honestly, the keys are straightforward I would consider not even setting them as instance variables at all and just checking for their presence


def error_json(reason)
{
status: 'error',
message: reason,
}
end
Comment on lines +55 to +60
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What status code are we returning in the error case? Is it still going to be 200 OK ?

Asking because it's something where we should probably want the status code to match the response status here, and in doing so, we could perform success / error handling on the basis of status code alone. i.e. we wouldn't need status at all, if the HTTP status code is enough of a signal.


def success_json(reason)
{
status: 'success',
message: reason,
}
end

def client
@client ||= DocAuthClient.client
end
end
end
38 changes: 17 additions & 21 deletions app/javascript/app/document-capture/components/documents-step.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import DeviceContext from '../context/device';
/**
* @typedef DocumentsStepValue
*
* @prop {DataURLFile=} front_image Front image value.
* @prop {DataURLFile=} back_image Back image value.
* @prop {DataURLFile=} front Front image value.
* @prop {DataURLFile=} back Back image value.
*/

/**
Expand Down Expand Up @@ -46,24 +46,20 @@ function DocumentsStep({ value = {}, onChange = () => {} }) {
<li>{t('doc_auth.tips.document_capture_id_text3')}</li>
{!isMobile && <li>{t('doc_auth.tips.document_capture_id_text4')}</li>}
</ul>
{DOCUMENT_SIDES.map((side) => {
const inputKey = `${side}_image`;

return (
<AcuantCapture
key={side}
/* i18n-tasks-use t('doc_auth.headings.document_capture_back') */
/* i18n-tasks-use t('doc_auth.headings.document_capture_front') */
label={t(`doc_auth.headings.document_capture_${side}`)}
/* i18n-tasks-use t('doc_auth.headings.back') */
/* i18n-tasks-use t('doc_auth.headings.front') */
bannerText={t(`doc_auth.headings.${side}`)}
value={value[inputKey]}
onChange={(nextValue) => onChange({ [inputKey]: nextValue })}
className="id-card-file-input"
/>
);
})}
{DOCUMENT_SIDES.map((side) => (
<AcuantCapture
key={side}
/* i18n-tasks-use t('doc_auth.headings.document_capture_back') */
/* i18n-tasks-use t('doc_auth.headings.document_capture_front') */
label={t(`doc_auth.headings.document_capture_${side}`)}
/* i18n-tasks-use t('doc_auth.headings.back') */
/* i18n-tasks-use t('doc_auth.headings.front') */
bannerText={t(`doc_auth.headings.${side}`)}
value={value[side]}
onChange={(nextValue) => onChange({ [side]: nextValue })}
className="id-card-file-input"
/>
))}
</>
);
}
Expand All @@ -75,6 +71,6 @@ function DocumentsStep({ value = {}, onChange = () => {} }) {
*
* @return {boolean} Whether step is valid.
*/
export const isValid = (value) => Boolean(value.front_image && value.back_image);
export const isValid = (value) => Boolean(value.front && value.back);

export default DocumentsStep;
6 changes: 0 additions & 6 deletions app/javascript/app/document-capture/context/upload.js

This file was deleted.

30 changes: 30 additions & 0 deletions app/javascript/app/document-capture/context/upload.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import React, { createContext } from 'react';
import defaultUpload from '../services/upload';

const UploadContext = createContext(defaultUpload);

/** @typedef {import('react').ReactNode} ReactNode */

/**
* @typedef {(payload:Record<string,any>,csrf?:string)=>Promise<any>} UploadImplementation
*/

/**
* @typedef UploadContextProviderProps
*
* @prop {UploadImplementation=} upload Custom upload implementation.
* @prop {string=} csrf CSRF token to send as parameter to upload implementation.
* @prop {ReactNode} children Child elements.
*/

/**
* @param {UploadContextProviderProps} props Props object.
*/
function UploadContextProvider({ upload = defaultUpload, csrf, children }) {
const uploadWithCSRF = (payload) => upload(payload, csrf);

return <UploadContext.Provider value={uploadWithCSRF}>{children}</UploadContext.Provider>;
}

export default UploadContext;
export { UploadContextProvider as Provider };
32 changes: 27 additions & 5 deletions app/javascript/app/document-capture/services/upload.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,30 @@
function upload(payload) {
return new Promise((resolve, reject) => {
const isFailure = window.location.search === '?fail';
setTimeout(isFailure ? reject : () => resolve({ ...payload, saved: true }), 2000);
});
/**
* Endpoint to which payload is submitted.
*
* @type {string}
*/
const ENDPOINT = '/api/verify/upload';

/**
* @type {import('../context/upload').UploadImplementation}
*/
function upload(payload, csrf) {
return fetch(ENDPOINT, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-CSRF-Token': csrf,
},
body: JSON.stringify(payload),
})
.then((response) => response.json())
.then((result) => {
if (!['success', 'error'].includes(result.status)) {
throw Error('Malformed response');
}

return result;
});
}

export default upload;
13 changes: 8 additions & 5 deletions app/javascript/packs/document-capture.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import AssetContext from '../app/document-capture/context/asset';
import I18nContext from '../app/document-capture/context/i18n';
import DeviceContext from '../app/document-capture/context/device';
import { Provider as AcuantProvider } from '../app/document-capture/context/acuant';
import { Provider as UploadContextProvider } from '../app/document-capture/context/upload';

const { I18n: i18n, assets } = window.LoginGov;

Expand All @@ -27,11 +28,13 @@ render(
endpoint={getMetaContent('acuant-sdk-initialization-endpoint')}
>
<I18nContext.Provider value={i18n.strings}>
<AssetContext.Provider value={assets}>
<DeviceContext.Provider value={device}>
<DocumentCapture isLivenessEnabled={isLivenessEnabled} />
</DeviceContext.Provider>
</AssetContext.Provider>
<UploadContextProvider csrf={getMetaContent('csrf-token')}>
<AssetContext.Provider value={assets}>
<DeviceContext.Provider value={device}>
<DocumentCapture isLivenessEnabled={isLivenessEnabled} />
</DeviceContext.Provider>
</AssetContext.Provider>
</UploadContextProvider>
</I18nContext.Provider>
</AcuantProvider>,
appRoot,
Expand Down
18 changes: 6 additions & 12 deletions app/services/idv/steps/doc_auth_base_step.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
module Idv
module Steps
class DocAuthBaseStep < Flow::BaseStep
include IdvSession

def initialize(flow)
super(flow, :doc_auth)
end
Expand Down Expand Up @@ -97,13 +99,13 @@ def post_back_image
result
end

def post_images
def post_images(front, back, selfie)
return throttled_response if throttled_else_increment

result = DocAuthClient.client.post_images(
front_image: front_image.read,
back_image: back_image.read,
selfie_image: selfie_image&.read,
front_image: front,
back_image: back,
selfie_image: selfie,
liveness_checking_enabled: liveness_checking_enabled?,
)
# DP: should these cost recordings happen in the doc_auth_client?
Expand Down Expand Up @@ -171,14 +173,6 @@ def mark_document_capture_or_image_upload_steps_complete
end
end

def liveness_checking_enabled?
FeatureManagement.liveness_checking_enabled? && (no_sp? || sp_session[:ial2_strict])
end

def no_sp?
sp_session[:issuer].blank?
end

def mobile?
client = DeviceDetector.new(request.user_agent)
client.device_type != 'desktop'
Expand Down
20 changes: 15 additions & 5 deletions app/services/idv/steps/document_capture_step.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,28 @@ module Idv
module Steps
class DocumentCaptureStep < DocAuthBaseStep
def call
response = post_images
if response.success?
api_upload = flow_session['api_upload']
response = (api_upload && api_upload['documents']) || post_form_images
Comment on lines +5 to +6
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can take advantage of Hash#dig here I think:

Suggested change
api_upload = flow_session['api_upload']
response = (api_upload && api_upload['documents']) || post_form_images
response = flow_session.dig('api_upload', 'documents') || post_form_images

handle_response(response)
end

private

def post_form_images
selfie = selfie_image ? selfie_image.read : nil
post_images(front_image.read, back_image.read, selfie)
Comment on lines +13 to +14
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do the nil-safe operator (or .try but I think rubocop prefers nil-safe these days)

Suggested change
selfie = selfie_image ? selfie_image.read : nil
post_images(front_image.read, back_image.read, selfie)
post_images(front_image.read, back_image.read, selfie_image&.read)

end

def handle_response(response)
if response.to_h[:success]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually our response objects have .success?

Suggested change
if response.to_h[:success]
if response.success?

save_proofing_components
extract_pii_from_doc(response)
extract_pii_from_doc(response) unless flow_session[:pii_from_doc]
response
else
handle_document_verification_failure(response)
end
end

private

def handle_document_verification_failure(response)
mark_step_incomplete(:document_capture)
notice = if liveness_checking_enabled?
Expand Down
1 change: 1 addition & 0 deletions app/services/idv/steps/selfie_step.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def handle_selfie_step_failure(failure_response)
failure(failure_response.errors.first, failure_response.to_h)
end

# rubocop:disable Naming/AccessorMethodName
def results_response
@results_response ||= DocAuthClient.client.get_results(
instance_id: flow_session[:instance_id],
Expand Down
3 changes: 3 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
# Twilio Request URL for inbound SMS
post '/api/sms/receive' => 'sms#receive'

# Image upload API for doc auth
post '/api/verify/upload' => 'idv/image_upload#upload'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes we try to stick to built-in rails method names/actions to be "RESTful"1 as much as we can, WDYT of renaming this to #create?


  1. I think "RESTful" is kind of a garbage term and can mean anything... in this case I'm just saying let's use the default names in rails for CRUD verbs


post '/api/service_provider' => 'service_provider#update'
match '/api/voice/otp' => 'voice/otp#show',
via: %i[get post],
Expand Down
Loading