-
Notifications
You must be signed in to change notification settings - Fork 18
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
Comments
Hey, sorry for the delay again! I don't get it. What do you mean with the following?
|
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 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. |
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. |
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. |
Sorry for the noise of stale bot. Hope I got this fixed now. |
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: []
}
// ...
} |
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 withauthorize({ ability: context => createAbility( ... ) })
To make this workaround easier, I wonder if the
authorize
hook should prefer to use its ability parameter overcontext.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!
The text was updated successfully, but these errors were encountered: