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

Handling "stale" ability on socket connection #33

Open
robbyphillips opened this issue Jun 3, 2021 · 6 comments
Open

Handling "stale" ability on socket connection #33

robbyphillips opened this issue Jun 3, 2021 · 6 comments
Labels
documentation Improvements or additions to documentation

Comments

@robbyphillips
Copy link

This might just be a documentation thing, but I thought I'd share because it was not obvious to me.

When setting up the ability as demonstrated in the docs -- attaching it to params after authentication, it seems that ability will only be updated when a new connection is established by the client. This can lead to confusing permission errors that then disappear on a page refresh.

At least I'm pretty sure that's what's happening.

I can workaround this issue by defining service-level hooks that remove context.params.ability then create a new ability with authorize({ ability: context => createAbility( ... ) })

To make this workaround easier, I wonder if the authorize hook should prefer to use its ability parameter over context.params.ability? That would make sense to me since more local/specific is typically how an override is done, but if you don't want to break the current behavior, maybe just a flag? Is there another way to handle this?

Thanks!

@fratzinger
Copy link
Owner

Hey, sorry for the delay again!

I don't get it. What do you mean with the following?

it seems that ability will only be updated when a new connection is established by the client

@robbyphillips
Copy link
Author

No worries!

tl;dr: If your ability might change due to some user interaction, the current example setup in the docs will not work nicely with the socket.io transport.


By "new connection" I meant the socket connection, but re-reading what I wrote above, I don't think that I did a very good job explaining the issue, so I'll try again with a bit of context :)

My ability factory function sometimes needs additional data to determine permissions. For example, Users may have Teams and they might have different permissions based on their Role in that team. Users may change or leave their Team at will.

We have a React client and use the socket.io transport for pretty much everything.

The "confusing permission errors" that I was referencing above happened when a User would leave or change a Team -- we could confirm that the change was persisted to the db, but the new data wasn't being used by the authorize hook. The most confusing part was that refreshing the page fixed the issue since the client shouldn't be able to affect its server side permissions object!

Eventually, I realized that the reason refreshing the page "fixed" the issue is because we were re-establishing the socket.io connection and making another call to the authentication service, which would build the new ability correctly.

So, it seems that the authentication service is only called once for the socket, and the ability is only created once. If your ability needs to change based on some user interaction with the app, then it needs to be attached to the request context somewhere else that will definitely get called per-request. That's the thing that was not obvious to me and might be nice to make a little note about in the docs.

@stale
Copy link

stale bot commented Oct 1, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Oct 1, 2021
@stale stale bot closed this as completed Oct 8, 2021
@fratzinger fratzinger reopened this Oct 9, 2021
@stale stale bot removed the wontfix This will not be worked on label Oct 9, 2021
@fratzinger fratzinger added the documentation Improvements or additions to documentation label Oct 9, 2021
@stale
Copy link

stale bot commented Dec 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 8, 2021
@stale stale bot closed this as completed Dec 16, 2021
@fratzinger fratzinger reopened this Dec 16, 2021
@stale stale bot removed the wontfix This will not be worked on label Dec 16, 2021
@fratzinger
Copy link
Owner

Sorry for the noise of stale bot. Hope I got this fixed now.
I want to support this, at least write a cookbook recipe in the docs.
@robbyphillips How did you fix this for your application?

@robbyphillips
Copy link
Author

It's a bit of kludge, but I'm really just stripping the default ability and recalculating it for every request on affected services.

// service-with-dynamic-permissions.hooks.ts

// remove the default ability 
const resetAbility = (context: HookContext) => {
  delete context.params.ability
  return context
}

// just a convenience wrapper
const makeAuthorize = () =>
  authorize({
    availableFields, // defined above somewhere for this service
    ability: getAbilityWithContext // ability factory
  })

export default {
  before: {
    all: [
      authenticate('jwt'),
      resetAbility
    ],
    find: [makeAuthorize()],
    get: [makeAuthorize()],
    create: [makeAuthorize()],
    update: [makeAuthorize()],
    patch: [makeAuthorize()],
    remove: [makeAuthorize()]
  },

  after: {
    all: [makeAuthorize()],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
  // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants