Skip to content

Angular-permission breaks state transition options #26

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

Closed
OverZealous opened this issue Dec 16, 2014 · 23 comments
Closed

Angular-permission breaks state transition options #26

OverZealous opened this issue Dec 16, 2014 · 23 comments

Comments

@OverZealous
Copy link
Contributor

Due to the fact that angular-permission completely stops and restarts every state transition, any options passed in (for example, via ui-sref-opts) are lost.

This means it's impossible to use angular-permission and do things like location: 'replace', because the option is lost.

@RafaelVidaurre
Copy link
Owner

Thanks for reporting this, this can actually be fixed:

$rootScope.$on('$stateChangeStart',
      function (event, toState, toParams, fromState, fromParams) {

The $stateChangeStart event provides this options object in toState.options according to docs, this means all we need to do is use it by changing

$state.go(toState.name, toParams, {notify: false}).then(function() { ... });

to something like

toOptions = toState.options;
toOptions.notify = false;
$state.go(toState.name, toParams, toOptions).then(function() { ... });

Think this would solve the issue you describe? (Haven't tested it myself)

Feel free to submit a PR if you can, I can otherwise fix this myself later this week when I'm free, though I'd priorize your PR since you discovered the problem and should get the credit for it.

Cheers and thanks

@OverZealous
Copy link
Contributor Author

I don't see the options property on toState. I dug into the debugger: it's just a literal reference to the state object definition.

As of right now, there appears to be no way to capture the options during a $stateChange event.

@RafaelVidaurre
Copy link
Owner

My bad I apparently read the docs wrong.

One option would be to specify state options via the data object. I don't like that permission is breaking this ui-router's original functionality, though if I can't find any other solution this would be the only way to go.

Something like:

$stateProvider
  .state('staffpanel', {
    url: '...',
    data: {
      permissions: {
        only: ['admin', 'moderator']
      },
      stateOptions: {
        location: 'replace'
      }
    }
  });

Any other ideas?

@OverZealous
Copy link
Contributor Author

The problem is those options are transient: they may only exist for a particular link (in my case, when toggling between different IDs within the same parent), but not when referenced from the outside.

For now, it's not critical, and UI-Router is supposed to have an all-new transition library soon, which will be a better place for Permission to hook.

I'm hoping in the short term they simply start passing the options along, you can vote for the issue to help bring attention to it. It's a much faster fix if they go that route, rather than waiting for a breaking-change release.

@OverZealous
Copy link
Contributor Author

The only other option I can think of would be to allow for synchronous permissions checks, which would complicate your library significantly. But the idea is, if the result of a permissions test is exactly true, then the change is allowed through. Anything else is promisified, and the change is handled as it currently is right now.

That seems really hacky, though.

@RafaelVidaurre
Copy link
Owner

Thanks for the reply, lets see if this issue can be addressed by the angular-ui team, otherwise we will have to re-check our options.

@abhinandankothari
Copy link

@Narzerus after using angular-permission in my application all the CREATE button on all forms(10 approx) have stopped working because soon as the resource is created reload: true is passed in state options as to reload and newly created resource is shown in list, Using following line:
$state.go($state.current, {}, {reload: true})

I have read the above comments and also posted on ui-router github issue and waiting for the solution.
For the meantime can you help me with a temporary/hacky solution ?

@RafaelVidaurre
Copy link
Owner

Since this issue depends on a problem with ui-router's $stateChangeStart I've created the following discussion. Please have a look at #51

@abhinandankothari
Copy link

@Narzerus 0.2.15 of Angular UI Router is released, when will you be releasing angular-permission to incorporate the changes ?

@RafaelVidaurre
Copy link
Owner

I will check the changelog and see what this implies before giving a
straight answer, but this should be updated soon, if anyone wants to send a
PR to speed things up I'll have a look
El El Thu, May 21, 2015 a las 4:22 AM, Abhinandan Kothari <
[email protected]> escribió:

@Narzerus https://github.com/Narzerus 0.2.15 of Angular UI Router is
released, when will you be releasing angular-permission to incorporate the
changes ?


Reply to this email directly or view it on GitHub
#26 (comment)
.

@asperling
Copy link

I rewritten the thing into my own module in a synchronous fashion and it works. That way you don't need to prevent the state transition in any case but those which aren't allowed. Is there any reason you used promises rather than the sync approach? I do like and use promises a lot but I don't see the need in this scenario)

@RafaelVidaurre
Copy link
Owner

Many people have roles which require a request to some backend
El El Mon, Jun 8, 2015 a las 4:42 AM, asperling [email protected]
escribió:

I rewritten the thing into my own module in a synchronous fashion and it
works. That way you don't need to prevent the state transition in any case
but those which aren't allowed. Is there any reason you used promises
rather than the sync approach? I do like and use promises a lot but I don't
see the need in this scenario)


Reply to this email directly or view it on GitHub
#26 (comment)
.

@ronanquillevere
Copy link

Hello,

I am facing exactly the same pb as @abhinandankothari and using ui-router 0.2.15, any improvements from here ?

By the way : thanks for the lib, appart from that it looked really great. But for me this bug is a showstopper

@RafaelVidaurre
Copy link
Owner

@ronanquillevere sadly as mentioned in the thread, this has to do with an issue on ui-router's end so there's not much we can do about it for now.

@ronanquillevere
Copy link

@Narzerus
I did not dive into the details of the pb but I though 0.2.15 might have fixed the pb, so I understand this is not case right ?

@RafaelVidaurre
Copy link
Owner

Actually I'm testing this right now, will respond in a moment

@asperling
Copy link

@Narzerus
I understand your point. But since ui-routers capabilities (it's not a bug in my opinion) do not support this kind of implementation, why not simply go sync? When ui-router is ready then an async version could be released depending on that version of ui-router and up.

As I see it one could more easily implement a workaround for the need of making requests during permission check (by caching those e.g.) as to find a way to cope with the issue of this thread. At least I guess a lot more people have said issue than the need to check back with a backend.

@RafaelVidaurre
Copy link
Owner

@asperling I'm not sure that's really the case, many people here need to query their back-end at specific points to get the actual session state, I cannot change such core behavior it would break everyone's code. I'm sorry if this is not the answer you were expecting, I'll try to find some way for this to work in your use-case

@ronanquillevere
Copy link

@Narzerus @asperling I do not have a strong opinion on the matter but I think making an asynch call to retrieve your permissions each time a route is called is not the best option in terms of performance. I personnaly do it only once when starting my app.

Would be interesting if someone could share his use case for an asynch call

@RafaelVidaurre
Copy link
Owner

I agree with that, still it would depend on the use-case. Caching will still work swiftly with promises, which is why I'm using it, its the best for both cases (If it weren't for this issue we are having)

@RafaelVidaurre
Copy link
Owner

Welp, about the "I'm testing this" it was actually meant as a response for a different issue.

@ronanquillevere
Copy link

@asperling would you have a fork with a sync version to share ?

@asperling
Copy link

@Narzerus I solved my case by integrating your great idea and some of your work in my own access module in a sync fashion and...
@ronanquillevere that's the reason why I have nothing to show for, at least no fork with a sync version.

But the main idea was to only stop propagation if a synchronous authorization check fails:

    $rootScope.$on('$stateChangeStart',
      function (event, toState, toParams, fromState, fromParams) {
        if (toState.$$finishAuthorize || !toState.data || !toState.data.permissions) {
          return;
        }
        var permissions = toState.data.permissions;
        if (permissions) {
          if (!AccessControl.authorizeSync(permissions, toParams)) {
            event.preventDefault();
            // If not authorized, redirect to wherever the route has defined, if defined at all
            var redirectTo = permissions.redirectTo;
            if (redirectTo) {
              $log.debug('ACCESS to %s DENIED, redirecting to %s', toState.name, redirectTo);
              $state.go(redirectTo, toParams);
            }
          }
        }
      }
    );

See https://github.com/Narzerus/angular-permission/blob/v0.1.7/src/permission.mdl.js for comparision.

Then you'll need to refactor all promise based functions into synchronous ones. As an example:

      authorizeSync: function (rightMap, toParams) {

        AccessControl._validateRightMap(rightMap);
        var authorizing;

        if (rightMap.only) {
          authorizing = AccessControl._findMatchingRightSync(rightMap.only, toParams);
        } else {
          authorizing = !AccessControl._findMatchingRightSync(rightMap.except, toParams);
        }

        return authorizing;
      }

instead of https://github.com/Narzerus/angular-permission/blob/master/src/permission.svc.js#L128

(_my module is called _AccessControl*, you will need to change that)

Maybe I can manage to build such a synchronous fork some time...

And @Narzerus, you're right, just switch back to sync would affect all of those that have already implemented it. Not to easy...

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

No branches or pull requests

6 participants