Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sebankid #2

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Sebankid #2

merged 6 commits into from
Jul 7, 2022

Conversation

mickhansen
Copy link
Collaborator

No description provided.

@mickhansen mickhansen requested a review from sgryt July 6, 2022 13:42
@mickhansen
Copy link
Collaborator Author

@sgryt hmm no netlify deploy preview yet, will investigate.

@netlify
Copy link

netlify bot commented Jul 6, 2022 •
edited
Loading

✅ Deploy Preview for criipto-verify-react-storybook ready!

Name Link
🔨 Latest commit e2afbaf
🔍 Latest deploy log https://app.netlify.com/sites/criipto-verify-react-storybook/deploys/62c6aca7a877c80009d8ed44
😎 Deploy Preview https://deploy-preview-2--criipto-verify-react-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sgryt
Copy link
Contributor

sgryt commented Jul 6, 2022

Not entirely sure what to expect - the login flow kind-a works in a laptop browser, but same-device on iOS does not switch back from the BankID app ?

@mickhansen
Copy link
Collaborator Author

Not entirely sure what to expect - the login flow kind-a works in a laptop browser, but same-device on iOS does not switch back from the BankID app ?

It should completely work on laptop and android, iOS still requires tweaking probably.

@sgryt
Copy link
Contributor

sgryt commented Jul 6, 2022

The BankID app starts and lets me complete a login, and I do get switched back to the browser, but it ends up with a "Native-app driven authentication is not done" message (but quickly this time)

@sgryt
Copy link
Contributor

sgryt commented Jul 6, 2022

Perhaps this comment from the SE BankID controller impl contains a clue ?

        // For iOS to work properly, it is critical that the app launches from 
        // exactly the same URL as it ends up redirecting to.
        [<HttpGet>]
        member this.Collect

?

@sgryt
Copy link
Contributor

sgryt commented Jul 6, 2022

can confirm that Chrome on Windows 10 ends up showing me a JSON structure with an id_token

@sgryt
Copy link
Contributor

sgryt commented Jul 6, 2022

The BankID app starts and lets me complete a login, and I do get switched back to the browser, but it ends up with a "Native-app driven authentication is not done" message (but quickly this time)

Switching back to the tab after about 30 mind shows me

"A technical error occurred in the communication with the native SE BankID provider. Please try again later.: Status code: BadRequest. \r\n{\"errorCode\":\"invalidParameters\",\"details\":\"No such order\"}"

So, the polling seems to be running

@mickhansen
Copy link
Collaborator Author

The BankID app starts and lets me complete a login, and I do get switched back to the browser, but it ends up with a "Native-app driven authentication is not done" message (but quickly this time)

Switching back to the tab after about 30 mind shows me

"A technical error occurred in the communication with the native SE BankID provider. Please try again later.: Status code: BadRequest. \r\n{\"errorCode\":\"invalidParameters\",\"details\":\"No such order\"}"

So, the polling seems to be running

Polling should only run in desktop case

@mickhansen
Copy link
Collaborator Author

mickhansen commented Jul 6, 2022 •
edited
Loading

Perhaps this comment from the SE BankID controller impl contains a clue ?

        // For iOS to work properly, it is critical that the app launches from 
        // exactly the same URL as it ends up redirecting to.
        [<HttpGet>]
        member this.Collect

?

We set redirect to encodeURIComponent(window.location.href) on iOS.

sgryt
sgryt reviewed Jul 6, 2022
View reviewed changes
src/components/SEBankIDSameDeviceButton/SEBankIDSameDeviceButton.tsx
refresh();
}, [refresh]);

// Continously fetch new autostart token if UI is open for a long time
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially trigger while the BankID app is in foreground on iOS?

Would explain the error message seen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgryt Hmm maybe if the onclick handler doesnt fire on iOS actually, could be it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgryt Latest commit disables the refetch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did not change the observed behavior, I’m afraid.

Copy link
Contributor

Choose a reason for hiding this comment

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

But could it be the other way around - in the sense that the displayed response is from before the flow is completed in the BankID app ?

Copy link
Contributor

@sgryt sgryt Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC, the reason we had to not poll in the HTML UI was because iOS lets JS continue to run when Safari is backgrounded.

Copy link
Contributor

Choose a reason for hiding this comment

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

When does setInterval run the first invocation of the specified function ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgryt We don't poll per say in the device flows, the intent is simply to refresh links if you're idle on the page so that the order doesn't expire.

We also automatically stop the refreshing as soon as the user lcicks on the button.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setInterval runs on the tail-end, so doesn't execute untill the first interval has passed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgryt Lets debug live with ngrok and slack-huddle tomorrow, will be easier.

@sgryt
Copy link
Contributor

sgryt commented Jul 7, 2022

@mickhansen Works on iOS now. If it also works on Android and Desktop, I vote to merge.

@mickhansen mickhansen merged commit 96b9815 into master Jul 7, 2022
@mickhansen mickhansen deleted the sebankid branch July 7, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgryt sgryt sgryt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mickhansen @sgryt