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.

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

Cancel login on "Same Device" gives blank screen #143

Closed
PeterOrneholm opened this issue Oct 29, 2019 · 7 comments
Closed

Cancel login on "Same Device" gives blank screen #143

PeterOrneholm opened this issue Oct 29, 2019 · 7 comments
Labels
bug Something isn't working
Milestone

Comments

@PeterOrneholm
Copy link
Member

Describe the bug
I obviously forgot to test this scenario, but if you cancel the login when choosing "Same Device" the page becomes empty, only showing the title "BankID".

What area is it related to
BankID

To Reproduce
Steps to reproduce the behavior:

  1. Go to the start page
  2. Click on "BankID (Same Device)"
  3. Click "Cancel"
  4. The page is cleared

Expected behavior
I've checked how Handelsbanken does it, and if you cancel on "Other device", they do like we do. Show the PIN input again. But when you cancel on "Same device", they take you back to the selection of schemes (Same / Other etc). I'd expect that to happen.

Screenshots
image

NuGet package version
Latest code in master.

Additional context
Suggestion would be that the server response for Cancel would include a redirectUrl, if set, the client would redirect there. That redirectUri would point to the "initial" page.

@PeterOrneholm PeterOrneholm added the bug Something isn't working label Oct 29, 2019
@PeterOrneholm PeterOrneholm added this to the 3.0.0 milestone Oct 29, 2019
@PeterOrneholm
Copy link
Member Author

I got an email with your comment @span, but seems to be gone here.

I think it is a valid scenario. We have an orderRef and I've tried it, and BankID does accept that we cancel it. To try it out, I used my PC but didn't allow the BankID app to open, then I cancelled it.

image

A scenario could be you choose "this device" but realize you actually wanted "other device" and want to cancel the first operation.

While on it, I realize we do get an error almost every time we cancel. That's because it tries to get one last status for that cancel, and the orderRef does not longer exists. I think we should check for cancel earlier, like this:

        function checkStatus(requestVerificationToken, returnUrl, loginOptions, orderRef) {
            if (loginIsCancelledByUser) {
                return;
            }

......

        }

@span
Copy link
Contributor

span commented Oct 29, 2019

Yes, sorry. I totally lost my head and was thinking of using qr codes with same device.

@PeterOrneholm
Copy link
Member Author

Yes, sorry. I totally lost my head and was thinking of using qr codes with same device.

Nah, still valid to question it, cause it is a different scenario :) Will look at this later today, should be a quick thing.

@span
Copy link
Contributor

span commented Oct 29, 2019

Let me know if you want me to have a look at it.

@PeterOrneholm
Copy link
Member Author

PeterOrneholm commented Oct 29, 2019 via email

@span
Copy link
Contributor

span commented Oct 29, 2019

Ok, added an initial naive implementation. It was more work than I expected. Hopefully I did not overengineer it :P

I added a new form field to map to a new view model property cancelReturnUrl. It is being sent back and forth together with the other parameters to the external account controller, bankid controller and the bankid api controller.

A few shortcuts if wanted:

  • Add a hash-argument to the location when going into the samedevice scheme. If cancelled, read the hash and redirect to it.
  • Access the bankid state cookie from the Login action in BankIdController. The cookie contains the redirect uri we want but it is currently only accessed in the authentication handler.

One argument for doing it as a separate property is that perhaps people want to redirect to different pages on success vs. cancel.

span pushed a commit to span/ActiveLogin.Authentication that referenced this issue Nov 5, 2019
Note that we still need the same check within the 'then' clause
since a check might have been started before the user cancels
and the response comes in after cancellation.

Ref ActiveLogin#143
span pushed a commit to span/ActiveLogin.Authentication that referenced this issue Nov 5, 2019
This commit adds a new property 'cancelReturnUrl' that is
used by the samedevice scheme to redirect the user back
to the initial page if they cancel an ongoing auth
process.

Currently it limits the cancelUrl to the samedevice
only but it is possible to allow the cancel url to be
set for the otherdevice scheme as well without too
much hassle.

Ref ActiveLogin#143
PeterOrneholm pushed a commit that referenced this issue Nov 11, 2019
* Do not check for another status if user cancelled

Note that we still need the same check within the 'then' clause
since a check might have been started before the user cancels
and the response comes in after cancellation.

Ref #143

* Use cancel return url for samedevice auth

This commit adds a new property 'cancelReturnUrl' that is
used by the samedevice scheme to redirect the user back
to the initial page if they cancel an ongoing auth
process.

Currently it limits the cancelUrl to the samedevice
only but it is possible to allow the cancel url to be
set for the otherdevice scheme as well without too
much hassle.

Ref #143

* Add testdata for cancel url and other form fields

* Fix redirect on cancel with qr codes

* Fixes for review on cancelling same device

* Re-add possibility to override cancel url

It was accidently removed ina previous commit.

* Add documentation on how to override the cancellation return url

* Fix bug in dictionary access for return urls on cancellation

* Restructure resolving of cancellation url

* Fix redirect bug in identity server example

* Enhances UI on cancel

* Always add cancelReturnUrl
@PeterOrneholm
Copy link
Member Author

Closed by #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants