LuckPerms

LuckPerms

41.4k Downloads

Groups are not removed from all players after deletion

TonyOneZero opened this issue · 17 comments

commented

What I have used
LuckPerms v4.3.7 on a Spigot-Server 1.8.8
Luckperms is connected to a MongoDB

/lp info
[LP] Running LuckPerms v4.3.7 by Luck.
[LP] -  Platform: Bukkit
[LP] -  Server Brand: CraftBukkit
[LP] -  Server Version:
[LP] -  git-PaperSpigot-"4c7641d" (MC: 1.8.8) - 1.8.8-R0.1-SNAPSHOT
[LP] -  Storage:
[LP] -     Type: MongoDB
[LP] -     Ping: 1ms
[LP] -     Connected: true
[LP] -  Messaging: None
[LP] -  Instance:
[LP] -     Static contexts: None
[LP] -     Online Players: 1 (0 unique)
[LP] -     Uptime: 19m 12s
[LP] -     Local Data: 1 users, 1 groups, 0 tracks

What I did:

The first thing I did was create a group.

/lp creategroup test
[14:25:27 INFO]: [LP] test was successfully created.

Then I added this group to a player as a parent.

/lp user TonyOneZero parent add test
[14:27:58 INFO]: [LP] tonyonezero now inherits permissions from test in context global.
/lp user TonyOneZero info
[14:28:59 INFO]: [LP] > User Info: tonyonezero
[14:28:59 INFO]: [LP] - UUID: ef678b44-7553-3074-becb-ba9546832b04 (type: offline)
[14:28:59 INFO]: [LP] - Status: Online
[14:28:59 INFO]: [LP] - Primary Group: default
[14:28:59 INFO]: [LP] - Parent Groups:
[14:28:59 INFO]: [LP] -    > default
[14:28:59 INFO]: [LP] -    > test
[14:28:59 INFO]: [LP] - Contextual Data:
[14:28:59 INFO]: [LP] -    Has contextual data: true
[14:28:59 INFO]: [LP] -    Applicable contexts: (world=world)
[14:28:59 INFO]: [LP] -    Prefix: None
[14:28:59 INFO]: [LP] -    Suffix: None
[14:28:59 INFO]: [LP] -    Meta: None

Then I deleted the group.

/lp deletegroup test
[14:30:18 INFO]: [LP] test was successfully deleted.

The problem:
Then I looked again at the parents of the user and the group test was still there.

/lp user TonyOneZero info
[14:33:57 INFO]: [LP] > User Info: tonyonezero
[14:33:57 INFO]: [LP] - UUID: ef678b44-7553-3074-becb-ba9546832b04 (type: offline)
[14:33:57 INFO]: [LP] - Status: Online
[14:33:57 INFO]: [LP] - Primary Group: default
[14:33:57 INFO]: [LP] - Parent Groups:
[14:33:57 INFO]: [LP] -    > default
[14:33:57 INFO]: [LP] -    > test
[14:33:57 INFO]: [LP] - Contextual Data:
[14:33:57 INFO]: [LP] -    Has contextual data: true
[14:33:57 INFO]: [LP] -    Applicable contexts: (world=world)
[14:33:57 INFO]: [LP] -    Prefix: None
[14:33:57 INFO]: [LP] -    Suffix: None
[14:33:57 INFO]: [LP] -    Meta: None

The above problem also leads to the following problem: #1252

commented

Any updates on that?

commented

That’s intended.

It’s intended because with the behavior the only change happening happens to the group. No other groups are affected. This is to make sure that you don’t accidentally change other permissions.

Bulkedit lets you remove the nodes easily.

commented

That’s intended.

It’s intended because with the behavior the only change happening happens to the group. No other groups are affected. This is to make sure that you don’t accidentally change other permissions.

Bulkedit lets you remove the nodes easily.

But the statement that the player as parent has the group test is wrong, because the group test doesn't exist anymore. I decide at the moment I delete the group that this group should not exist any longer. If at any time I decide to make a new group called test, all players who had test as parents before will automatically have that group as parents again, which I might not want at all.

So every time I really want to delete a group I would have to use Bulkedit every time.

I don't really think that's the way it should be.

commented

The reasoning in short terms is data consitency. All other commands fulfil the principle. And it makes a lot of sense to do so.

The only thing that would be making sense (in light of how the plugin works and what promises (code and data wise) it makes) is adding a “I know I’m violating data consistency, remove it from the others as well”.

The same should be implemented to the rename command too.

commented

I'm happy to be persuaded otherwise, but my current opinion is that this is shouldn't be changed. :)

commented

I kinda like that removing the actual group memberships has to be a specific extra step.

The benefit of keeping it as "an extra required step using bulkupdate" is that the process for doing that is quite involved - you have to confirm the operation before it executes, and it has to be done via console.

"Big permission changes" - i.e. changes that will affect a lot of user data should have warnings beforehand imo.

commented

As I’ve said before I think a flag would be a good compromise.
Still nothing you’d do by accident and still easy and convenient to use.

commented

I agree with that.

commented

The same flag should be added to the rename command.

commented

@lucko
I agree with your idea.
But I personally still can't understand why groups after deletion won't be removed from all players who had them as parents before. If the group no longer exists and therefore no longer the rights that were in the group. Is the statement that a user has this deleted group as a parent wrong.
In my opinion you can only say that he had it as a parent before, but he can't have it anymore, because the group doesn't exist anymore.

I can understand that you should not delete groups by mistake. But you normally don't delete groups by mistake. You enter the command /lp deletegroup and you say that you want to delete a group. I could understand maximum if you were asked if you really wanted to do it. But even that is too much. The command /lp deletegroup is to delete groups or do I understand something wrong?

So I see it more as an issue than a suggestion.

I don't want to force you to accept my point of view, but I want to understand your point of view and unfortunately I don't understand it yet.

commented

The thing is what happens when you accidentally delete the wrong group?

With the current behavior you just add it back. No harm done.

It might be a good idea to force a confirmation here too, when the flag is set.

commented

@BrainStone
well, if I accidentally deleted a group I'd have to reset all permissions too.

You're right but if I want to REALLY delete a group which is often the case I wonder as a plugin user why Users still has the group as parent.

I think it's good to think about possible misbehaviour of the user, but still the normal case should be in the foreground. And I think that as it is currently the normal case will get into the background.

@lucko
How about removing the deleted group after 1 minutes from all players who had it as parent?

The whole thing leads for example to the following error: #1252

commented

An alternative option would be using a flag to not delete/rename the group everywhere, as that would fit in with the command being simplest/shortest for the most common use case.

However I still believe there should be a flag option no matter which way round.

commented

An alternative option would be using a flag to not delete/rename the group everywhere, as that would fit in with the command being simplest/shortest for the most common use case.

Seems reasonable

commented

/edited added:
The above problem also leads to the following problem: #1252

commented

Moved to the above issue ^

commented

Bringing this back up for v5 @lucko