Skip to content

LG-12306: selfie standardflow check#10112

Merged
dawei-nava merged 27 commits intomainfrom
dwang/LG-12306_selfie_standardflow_check
Feb 27, 2024
Merged

LG-12306: selfie standardflow check#10112
dawei-nava merged 27 commits intomainfrom
dwang/LG-12306_selfie_standardflow_check

Conversation

@dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Feb 20, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12306

🛠 Summary of changes

In document capture controller precondition, prevent entering document capture controller from hybrid handoff if selfie enabled. Based on @amirbey's work.

A config item doc_auth_selfie_desktop_test_mode to allow enable selfie on desktop. Default is false for production, true for local dev and testing.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.
Note: for local set doc_auth_selfie_desktop_test_mode: false

  • Step 1: Login with SP requiring biometric
  • Step 2: On desktop, entering doc auth flow
  • Step 3: On hybrid handoff screen, choose upload from computer
  • Step 4: It will be redirected back to hybrid handoff screen
  • Step 6: Logout and Login with sp not requiring biometric.
  • Step 7: On desktop, entering doc auth flow
  • Step 8: On hybrid handoff screen, choose upload from computer
  • Step 9: It should proceed to document capture screen.

@dawei-nava dawei-nava marked this pull request as ready for review February 20, 2024 15:01
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏿

@amirbey
Copy link
Contributor

amirbey commented Feb 20, 2024

Looks great @dawei-nava 👍🏿

i made a few suggestions on the subbing for forcing selfie capture to be more inline with how we check for selfie using decorated_sp_session in the controller. Also, could be great if you could add some feature testing 👍🏿

@dawei-nava
Copy link
Contributor Author

Looks great @dawei-nava 👍🏿

i made a few suggestions on the subbing for forcing selfie capture to be more inline with how we check for selfie using decorated_sp_session in the controller. Also, could be great if you could add some feature testing 👍🏿

@amirbey , still working on some tests.

@dawei-nava dawei-nava force-pushed the dwang/LG-12306_selfie_standardflow_check branch 2 times, most recently from cfb4817 to 4aaa529 Compare February 21, 2024 20:22
Copy link
Contributor Author

@dawei-nava dawei-nava Feb 21, 2024

Choose a reason for hiding this comment

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

Maybe it worth considering to inject sp_ession/decoreate_sp_session or even the controller itself into step preconditions, since we may need more and more context information to make decisions.

Comment on lines 111 to 112
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏿

@amirbey
Copy link
Contributor

amirbey commented Feb 22, 2024

LGTM @dawei-nava 👍🏿
before approving ... i am just waiting on the outcome of a question posed during today's refinement as to whether we add an AC as to whether or not to enforce this in local/dev

@dawei-nava
Copy link
Contributor Author

dawei-nava commented Feb 23, 2024

LGTM @dawei-nava 👍🏿 before approving ... i am just waiting on the outcome of a question posed during today's refinement as to whether we add an AC as to whether or not to enforce this in local/dev

@amirbey , added a config flag to allow desktop selfie mode. Set to true in dev and testing.

@dawei-nava dawei-nava force-pushed the dwang/LG-12306_selfie_standardflow_check branch 3 times, most recently from 7b00f29 to 88dc95c Compare February 23, 2024 16:31
Copy link
Contributor

Choose a reason for hiding this comment

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

@dawei-nava - what is the intended outcome of the flag in the hybrid handoff precondition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbey Was talking with Jack from Team Joy, it is kind of complicated when involves the self.select_remote part, the desktop_selfie_test_mode_enabled? is to allow we land on hand off page for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbey , removed as discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏿

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏿

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if I'm missing a setup step, but when I check out this branch and try running through the testing steps, I did not see the expected result.

Specifically, on desktop, I was not re-directed to the hybrid handoff page. Instead I was able to continue to the upload page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@night-jellyfish , you have to set doc_auth_selfie_desktop_test_mode: false

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh okay. I missed that note somehow. Thanks! I can try again later today if you'd like (quite a few meetings this am) but don't need to block it.

@dawei-nava dawei-nava force-pushed the dwang/LG-12306_selfie_standardflow_check branch from 9f081b1 to 23c5fe8 Compare February 27, 2024 14:23
@dawei-nava dawei-nava force-pushed the dwang/LG-12306_selfie_standardflow_check branch from 23c5fe8 to 21e1da0 Compare February 27, 2024 14:25

context 'opted in to ipp flow' do
before do
allow(IdentityConfig.store).to receive(:doc_auth_selfie_desktop_test_mode).
Copy link
Contributor

Choose a reason for hiding this comment

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

@dawei-nava - do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbey removed.

Comment on lines +855 to +856
allow(IdentityConfig.store).to receive(:des).
and_return(true)
Copy link
Contributor

@amirbey amirbey Feb 27, 2024

Choose a reason for hiding this comment

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

where can I find des ?... i am unable to locate this 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbey , oooh, my IDE completion is too aggressive.

end

let(:happy_selfie_path_events) do
let(:happy_mobile_selfie_path_events) do
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏿

allow_any_instance_of(FederatedProtocols::Oidc).
to receive(:biometric_comparison_required?).
and_return({ biometric_comparison_required: true })
and_return(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏🏿

before do
allow_any_instance_of(FederatedProtocols::Oidc).
to receive(:biometric_comparison_required?).
and_return({ biometric_comparison_required: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and_return({ biometric_comparison_required: true })
and_return(true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbey corrected.

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏿

@dawei-nava dawei-nava merged commit bdefaab into main Feb 27, 2024
@dawei-nava dawei-nava deleted the dwang/LG-12306_selfie_standardflow_check branch February 27, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants