search

kandashi / Active-Token-Lighting

MIT License
23 stars 25 forks source link

User XXX does not have permission to update token XXXX #126

Closed tposney closed 1 year ago

tposney commented 1 year ago

When an ATL active effects is applied to an NPC (for example) each non-gm player client gets the message User XXX does not have permission to update token XXXX

This is caused by line 404 of activeLighting.js

        await canvas.scene.updateEmbeddedDocuments("Token", updateMap)

I think it should/could be 

        const firstGM = game.users?.find(u => u.isGM && u.active);
        if (game.user.id === firstGM.id) await canvas.scene.updateEmbeddedDocuments("Token", updateMap)

I think the canvas update only needs to be done once, rather than on each client doing the applyEffects code.

I had a further play around and from what I can see the whole apply active effect method only needs to run on a single GM client - since all updates are propagated to each client. So the check for firstGM could be at the start of the applyEffect method as

        const firstGM = game.users?.find(u => u.isGM && u.active);
        if (game.user.id !== firstGM.id) return;

Environment:

Additional context Add any other context about the problem here.

WindDragonLord commented 1 year ago

Very useful!Thanks!

MaxPat931 commented 1 year ago

The adjustment to line 404 fixed it for me, thanks @tposney !

dineshm72 commented 1 year ago

I put the second set of lines in at line 276 and similarly had good results, thanks!

(Note that you then don't do the first set of lines.)

kaelad02 commented 1 year ago

The one issue I have with the fix is there's a setting that allows ATE to work without a GM logged in. If we changed it to only happen by the GM then it wouldn't work in that case. Instead, what if it's done by the person who triggered the active effect change (e.g. the userId sent in the createActiveEffect hook)? I know not all of the calls to apply active effects to the token are done by AE changes (e.g. the canvasReady hook) so those we could use the first GM.

I'll try it myself after finishing the "no prototype token" rework, but wanted to get your opinion on this too.

kaelad02 commented 1 year ago

To be a bit more concrete, this is what I was thinking of:

Hooks.on("createActiveEffect", async (effect, options, userId) => {
  if (userId !== game.userId) return;
  // remaining code including the applyEffects call
})
tposney commented 1 year ago

That should be fine, since if the user IDs match the user must have had permission to make the change since it is triggering on the post supply effect call.

On Sat, Apr 1, 2023, 09:19 Chris Seieroe @.***> wrote:

To be a bit more concrete, this is what I was thinking of:

Hooks.on("createActiveEffect", async (effect, options, userId) => { if (userId !== game.userId) return; // remaining code including the applyEffects call})

— Reply to this email directly, view it on GitHub https://github.com/kandashi/Active-Token-Lighting/issues/126#issuecomment-1492561344, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCHH73IWYOG7D3PCJFESZDW643Q7ANCNFSM6AAAAAAUUT2NFQ . You are receiving this because you were mentioned.Message ID: @.***>

kaelad02 commented 1 year ago

@Larkinabout Looks like the changes in #121 introduced this bug. I've had a chance to look over ATE's code more and I think I understand why the "is GM" check was there in the first place: as a way to only do the update once. It was more about making sure the update only ran once than a permissions thing.

I also realized there's an existing user permission, Configure Token Settings, that determines whether the user can configure their own token from the token HUD. They can still make changes via macros, but just not the UI. I tweaked the code to use it instead of the setting introduced in #121 but I'm still not sure I like this any better. Here's some of that code:

// this replaces getAllowed
function shouldPerformUpdate(userId) {
    // if no user triggered this, then only run on first GM
    const firstGM = game.users.find(u => u.isGM && u.active)?.id;
    if (!userId) return firstGM;

    const userCanConfigToken = game.users.get(userId).can("TOKEN_CONFIGURE");
    if (game.userId === userId) {
        // if I'm the user that triggered the update and I can configure tokens
        if (userCanConfigToken) return true;
        // show error if there's no GM to do the update for us
        if (!firstGM) atlDisabledNotification();
    } else if (game.userId === firstGM && !userCanConfigToken) {
        // if I'm the first GM and the user that triggered this can't configure tokens
        return true;
    }

    return false;
}

// replace this
if (!isAllowed) {
    atlDisabledNotification();
    return;
}
// with this
if (!shouldPerformUpdate(userId)) return;

It works, making sure that the updates only happens once and avoids this bug. But... I'm really wondering if this is worth it at all. This seems like a lot of fragile code for very little payoff. I know it might be nice for a player to tweak their ATE active effects w/o the GM present but I don't know that it's worth it. Would you be okay if I just reverted #121 and went back to only doing these updates by a GM?

Larkinabout commented 1 year ago

It hasn't affected me directly since the first instance, so I'd say it's your choice. I'd probably still suggest keeping some sort of message for users so they don't waste time trying to figure out why their effects are no longer working (like I did).

Personally, I'd probably favour keeping everything related to the triggering user through the entire flow, even with or without the module setting, as that feels simpler to understand, even though it might not be simpler code. But, without the module setting, it won't really matter either way.

kaelad02 commented 1 year ago

I changed the hooks to be done by the user that triggered them. That'll make sure they're only done once, as intended, and by a user that has permission. If the user has permission to change an active effect on an actor, they can change a related token.

kaelad02 commented 1 year ago

version 0.5.2 has this fix