ActiveLogin / ActiveLogin.Authentication

Support Swedish BankID (svenskt BankID) authentication in .NET. Unofficial package, not created by BankID.
https://activelogin.net
MIT License
216 stars 75 forks source link

Support for controlling browser refresh behavior on iOS #423

Closed altenstedt closed 9 months ago

altenstedt commented 1 year ago

PR #414 that originates from issue #339 improves support for login from native iOS apps that are using an embedded web view like ASWebAuthenticationSession. However, when the user returns from the BankID app, the default implementation in BankIdLauncher will refresh the browser on iOS which does not work for this scenario.

I would like to override the refresh behavior so that I do not have to implement IBankIdLauncher (basically as stated in https://github.com/ActiveLogin/ActiveLogin.Authentication/issues/339#issuecomment-1707151117)

PeterOrneholm commented 1 year ago

Thanks! Very god you brought this up :) I like your PR, but would like to see it implemented slightly different.

Instead of having a new interface for this, I would like it to be another method on the existing interface (IBankIdLauncherCustomAppCallback.cs). That would mean a breaking change, but if we use the "new" feature of default interface methods we could fallback to the default behavior.

I would suggest a method called (something like) DeviceWillReloadPageOnReturnFromBankIdAp that could return an enum with values like Default, False, True. By default it will use the default implementation based on the detected device.

As these things usually are coupled, I think this is the bestr method. Do you agree?

altenstedt commented 1 year ago

Of course. I have updated the PR with a new commit that implements your suggested design.

I felt that keeping the default implementation of GetDeviceWillReloadPageOnReturnFromBankIdApp in the BankIdLauncher class was more clear, compared to moving the code to a default implementation in the interface.

Please note that the new enumeration contains documentation about the default behaviour. You might not want that, but I felt it was a reasonable piece of information to expose to the developer when intellisensing the value.

What do you think? This is pretty untested, just to get your feedback.