search

SpoonX / aurelia-authentication

Authentication plugin for aurelia.
http://aurelia-authentication.spoonx.org
MIT License
90 stars 60 forks source link

Popup Not Working As Expected #366

Closed louislewis2 closed 3 years ago

louislewis2 commented 7 years ago

Hi,

We recently found an issue with the way in which the popup determines when to close. Using Windows Server, IIS and virtual directories. The server hosts more than 1 deployed solution. virtual directories are used to partition deployed instances.

The server side might reside at: localhost/web1/ The client might reside at: localhost/ui1/

The virtual directory wont be part of the host string as you are looking for in popup.js https://github.com/SpoonX/aurelia-authentication/blob/master/src/popup.js#L65

This means the moment the user enters their credentials and submits the form, then the code outlined above sees the operation as complete and closes the popup.

The general flow is that the popup should open the login url, which it does. the user then enters credentials, if they are correct the server redirects to the the authorization page, which is where the user can choose whether to authorize the application or not. Due to the way popup.js is working this last part never happens, because the popup closed directly after the credentials are submitted.

In most cases OpenId Connect has application clients which it knows about in the backend. They are registered / stored with redirect urls, which is where the server will redirect to, if authentication and authorization was successful.

I think it would be beneficial if you provide a parameter which a user of your library can set. popup.js will then compare this variable to the current address in the popup and the outcome of that comparison will determine whether the popup should close or not. I dont think a new variable needs to be added either, simply using redirectUri should work as expected.

RWOverdijk commented 7 years ago

@louislewis2 Thank you for your extensive description. It sounds to me that your current solution is very much an edge-case caused by an irregular setup that we haven't recognized in our solution. I'm sorry to read that you're having problems matching those to this plugin and would love for it to work for you.

If you can provide a BC fix, I'd be more than willing to accept a PR. Sounds like a nice addition! Please keep our contributing guidelines in mind, and reference this issue in your PR.

Thank you once again.

louislewis2 commented 7 years ago

@RWOverdijk Thanks for the reply. Okay I have forked and cloned and started going through the source. The primary change is in popper.js, because there is more than 1 way to implement this, I think it would be wise to discuss some options here first.

Based on my first pass through the source i came up with the following idea. within Popup.js

To the open method. Add another optional parameter called returnUrl. This will be stored as a local variable.

Within the pollpopup() method. line 65 you have the if statement to determine whether to accept resolve or reject the promise. My idea is to first check whether the returnUrl has been set, and if so it will be tested against the popup location, if that variable was not set it should then work as before.

If this is good so far, then I will start back tracing the code to see where calls to the open method will need to be updated and determine whether or not the idea will work throughout the code base.

You having more experience within your own source, I am hoping that you can look into the idea noted above and at least have an idea whether it could be a viable solution.

RWOverdijk commented 6 years ago

I think it's a pretty big change for such a problem. Wouldn't it simply be enough to match the auth path instead? Listen for host?