search

userapp-io / userapp-angular

AngularJS module that adds user authentication to your app with UserApp.
https://userapp.io
MIT License
233 stars 84 forks source link

make access control of routes/states more flexible #3

Closed jack-kerouac closed 10 years ago

jack-kerouac commented 10 years ago

By introducing an authorizationRequiredHandler and an accessDeniedHandler, reaction to these two types of access control violations is more flexible. authCheck complements hasPermission by a more flexible mechanism for access-checking of routes/states. I cleaned up some other route/state transitions when logging in/out.

As a result, the login form could now be displayed in a popup (no dedicated login state required anymore), as well as a notification about access denial. Logging in and out does not necessarily change the route/state anymore (if access rights allow that) but just reload the current state.

What do you think?

timothyej commented 10 years ago

Looks awesome, thanks! But there seem to be a bug with transitioning to the default route after login, at least in my tests. I will take a look at this. Also, is there any specific reason why you do a reload() at reset() and loadUser(), instead of wrapping the new logic in a function?

Again, thanks!

timothyej commented 10 years ago

The bug seem to be related to the reload(), because after login, it just reloads the state/route. A solution would be to always redirect to the default route if the login route is accessed when logged in.

jack-kerouac commented 10 years ago

You are right. I replaced the reloading of the route/state by a re-check of access rights. See the latest change.

jack-kerouac commented 10 years ago

Considering the transition to the default route after login: I removed it which might have been a bad idea since it changes the default behavior. The reason I did this is that I think this behavior is to rigid: If the login form is just a popup, not a state, I do not want a state change happening after login. Also, if a user accesses a state for which he needs to log in, there should be no redirect to the default route afterwards, but to the state the user tried to access. So in this case, the transition to the default route is not required as well. In general, I think that there should be no state transitions in the library at all.

IMHO, state transitions in reaction to login/logout should be implemented through listeners to the user.login and user.logout event.

So what to do? Risk an incompatible behavior (remove transitioning to the default route), or readd it?

timothyej commented 10 years ago

Sorry for the delay.

Yes you are right, there might be a lot of different use cases, but it would break all the existing apps that are using this library. So my suggestion is maybe to have transitions as default, and if it is overridden then they won't happen.

So if the listeners are used, no transitions are made, else they are as default. What do you think of that?

timothyej commented 10 years ago

I will merge this and then fix the last fixes on the main branch.

timothyej commented 10 years ago

I added an authenticationSuccessHandler that gets invoked after a successful login. If this function returns true (default) it will automatically redirect to the default route.

Please let me know what you think, and help me test it before I release a new version.

Thanks! This module is getting pretty awesome ;)

jack-kerouac commented 10 years ago

Hi.

Now I am sorry for the delay. The solution looks good! I tried it and it works for me. But why is the transition made based on the return value of the authenticationSuccessHandler and not just by the default handler?

timothyej commented 10 years ago

I'm sorry for the delay as well ;)

I just made it that way to keep it easy for the user. If someone would like to override the authenticationSuccessHandler they could do that and still keep the default behavior if wanted. What do you think of that design?

Thanks!

jack-kerouac commented 10 years ago

I think your point is valid but I would have preferred the authenticationSuccessHandler to transition the state. This way, it would be the same behavior as with the other handlers I introduced.

On Tue, May 13, 2014 at 11:36 PM, Timothy E. Johansson < notifications@github.com> wrote:

I'm sorry for the delay as well ;)

I just made it that way to keep it easy for the user. If someone would like to override the authenticationSuccessHandler they could do that and still keep the default behavior if wanted. What do you think of that design?

Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/userapp-io/userapp-angular/pull/3#issuecomment-43016379 .

timothyej commented 10 years ago

Yes, you are right, it should be consistent. I will have an extra look at this tomorrow.

Btw, I've made some changes to the input parameters to the handlers which could break your existing code. Could you try the latest version before I publish it? You can see the changes here: https://github.com/userapp-io/userapp-angular/commit/db7b5d13a444efb759810fd9ec169ca21df6e207

Thanks!

timothyej commented 10 years ago

Hi again,

I've changed the default behavior of authenticationSuccessHandler to do the transition, as you suggested. It's more consistent that way.

Timothy