-
Notifications
You must be signed in to change notification settings - Fork 21
fix: backgroundlock path state #337
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jan <[email protected]>
// Encode the path for redirection after unlock | ||
const encodedRedirect = TypedArrayEncoder.toBase64URL(TypedArrayEncoder.fromString(lastPath)) | ||
router.replace(`/authenticate?redirectAfterUnlock=${encodedRedirect}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the thing we want to fix is to handle a deeplinkg, but the wallet is closing the wallet on becoming active, so the deeplink isn't handled.
This may (not sure) fix the issue of a deeplink, but in case you were in a flow and went to the background, this won't work and require us to refactor a lot of of the logic, and will lead to bugs 🤔
So i think we need to know here whether we are handling a deeplink or not. if not, we should just go to authenticate, if we do, we need to extract the deeplink url and redirect to that aftewards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to always require authentication when using a deeplink (easiest solution). The +native_intent runs first thing the app launches/comes alive, so there isn't a nice way to combine this with the current BackgroundLockProvider (e.g. not requiring authentication when authentication has been done < 60 seconds).
It think it is possible, but not sure if worth the hassle. Probably have to make a placeholder entry page for the deeplink routes where you check the last active time or something. Would also result in an extra spinner showing.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the flow bug with the background lock; you could handle inactive differently than background. E.g. when the modal opens to do the authorization code flow we have a state of inactive, and we could allow for a longer time (e.g. 2-3 minutes) then when you manually close the app into the background (1 minute).
I think if you want to keep the whole state, you'd have to make something of an authentication overlay where you enter your pin. With our current implementation I don't think it's possible as we unmount the actual route you're in.
Let's wait with merging, as I think this will break the mDL flow with the AusweisApp (since that requires native deeplinking midflow) |
FUN-513
This doesn't work for requests right? because we can't receive a request again, that leads to an error.
Any way around that?