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

Ensure parent permissions are evaluated only once #345

Conversation

joeljeske
Copy link

This fixes Goal 1 marked #338.

The issue was, that if I had a state hierarchy like,

$stateProvider
  .state('root', {
    ...
    data: {
      permissions: {
        only: 'isLoggedIn'
      }
    }
  })
  .state('root.child', {
    ...
  });

then, upon $state.go('root'), the isLoggedIn permission would evaluate twice, once for the parent and once for the child.

This was due to UI-Router making the data property of states prototypically inherit from their parent. Thus if a child does not specify its own permissions property, then it would use its parent property.

This was easily fixed by making sure we would only use the permissions from a state that hasOwnProperty('permissions')

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.324% when pulling d947c33 on joeljeske:feature/multiple-permission-evaluation into 7263962 on Narzerus:development.

@@ -56,7 +56,8 @@ function PermStatePermissionMap(PermPermissionMap) {
*/
function areSetStatePermissions(state) {
try {
return !!state.data.permissions;
// We check for hasOwnProperty here because ui-router lets the `data` property inherit from its parent
return Object.prototype.hasOwnProperty.call(state.data, 'permissions') && !!state.data.permissions;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using only hasOwnProperty should be sufficient I think. No need to recheck by calling property itself afterwards.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I generally agree except if someone uses null as the permissions value. Edge case though, I guess.

@masterspambot masterspambot merged commit 542a166 into RafaelVidaurre:development Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants