fnakstad / angular-client-side-auth

One way to implement authentication/authorization in Angular applications
http://angular-client-side-auth.herokuapp.com/
MIT License
1.63k stars 346 forks source link

Titles for Access Levels show last declared role in array #49

Closed iotaweb closed 10 years ago

iotaweb commented 10 years ago

First off - thanks so much for this project - it is greatly appreciated!

Not sure of this is an issue or not, but when listing accessLevels, I notice that the title seems to be the last role declared in the array, e.g.

Config:

        accessLevels: {
            'public': '*',
            'anon': ['public'],
            'user': ['user', 'userpro', 'admin'],
            'userpro': ['userpro', 'admin'],
            'admin': ['admin']
        }

Resulting accessLevels:

{ public: { bitMask: 15, title: '*' },
  anon: { bitMask: 1, title: 'public' },
  user: { bitMask: 14, title: 'admin' },
  userpro: { bitMask: 12, title: 'admin' },
  admin: { bitMask: 8, title: 'admin' } }

As you can see, admin is the tile for the last three access levels. I was wondering if thIs was deliberate or whether it was a bug?

Thanks, Rob

fnakstad commented 10 years ago

Hi Rob! Sorry my reply is so late. This seems like unintended behavior, so I'll take a look at it this weekend. Thanks for reporting :)

mprzydatek commented 10 years ago

Hi - i noticed the same behavior, the order of declaring roles in access levels is important. This is because the buildAccessLevels function defined in routingConfig is (incorrectly) taking the last role declared when setting up the "title" attributes. What's more important, because the Auth.isLoggedIn() method is making decisions based on titles comparisons, it is allowing users with the "user" access level, to enter resources as if they had "admin" role (at least in the original code that resides in the repo, at the time of writing this). Nevertheless, great job with coming up with the concept for client-side auth. Cheers.

fnakstad commented 10 years ago

Thanks for the detailed analysis! I've been busy working on the ui-router migration lately, but since this seems like a really serious bug I'll get on this right away.

iotaweb commented 10 years ago

Thanks Frederick! One question, in my app I now have the following roles:

roles: [
    'public',
    'user',           
    'userpro',
    'users',            
    'admin'
]

... and access levels:

accessLevels: {
    'public': '*',
    'anon': ['public'],
    'user': ['user'],
    'userpro': ['userpro'],
    'users': ['user', 'userpro', 'admin'],
    'admin': ['admin']
}

As you can see, the title for one of my access levels is users which is not actually a role. So will your fix return the key for the title as opposed to one of the roles as it currently does? (hope this makes sense).

Cheers, Rob

fnakstad commented 10 years ago

Yes, that was my initial thought. However, upon closer inspection I think I might delete the title property completely from the accessLevels object. The "title" property mprzydatek mentioned (the one used in the "isLoggedIn" function of the Auth service) is actually not from the accessLevels object but from the userRoles object. The title property of the userRoles object is set correctly, so it seems we don't have a bug here after all. So the "title" property of the accessLevels object was added as a mistake as there is no use for it in any of the other services or directives. I will push out a new commit removing title from the generated accessLevels in a few minutes. That is unless you guys see a good use for it in your own applications.

Oh, and so sorry for neglecting this issue for so long Rob. It flew off my radar since I was busy with other things, so I'm very sorry it took so long.

iotaweb commented 10 years ago

Hah, you're right - I checked my code and I only use role.title in my app! So I have no objections to removing the Access Level title property (unless someone else is using it). And no worries about the delay, it hasn't held me up a bit - just noticed it in passing :)

BTW - this repo is proving very useful for me - have integrated with Hapi.js and using Stormpath for user account management. Your cookie on load to set the user role is inspired!

fnakstad commented 10 years ago

Happy to hear that :) Let me know if/when your project is up and publicly available!

mprzydatek commented 10 years ago

role title is still being used in Auth service (isLoggedIn function).

fnakstad commented 10 years ago

Yes, but the title property used is that of the userRoles object, not the accessLevels object. The titles are calculated correctly for the userRoles object, so this is not a problem. The original problem here was that for some reason accessLevels also received a title property (which was calculated incorrectly) when there is no tangible use for a title property on the accessLevels object. This is the title property that as removed in the commit I mentioned.

mprzydatek commented 10 years ago

OK, got it. Thanks Frederik. Take care.