[Suggestion] Bypass the purge with permission

[foxi69]

I think its would be nice if I can add a permission to my staff group whos accounts never purged then :)

[ljacqu]

Would a list of names in the config also be a possibility? I think it would be easier to implement.

[foxi69]

hmm not bad, but I think the permission is would better because the player names can changed

[sgdc3]

+1 permission ;)

[foxi69]

but If u can create it for usernames, that would be nice also:)

[sgdc3]

@ljacqu @games647 in order to accomplish this we need a way to check the permission of an offline player with a certain nickname. Should we add it to the perm manager?

[ljacqu]

One question I have for this is how many perm managers support this.
There's a refactoring story for the perm manager that would go lovely with this btw, but it's a big one

[sgdc3]

almost every perm manager has a deprecated pre uuid method

[ljacqu]

In that case - sounds good! Can I have until the weekend to do some refactoring on the perms manager? It's one huge block of code that needs to be structured a little. Seems like I can't interest anyone into refactoring it :P

[foxi69]

so what happend? :D

[EbonJaeger]

@ljacqu, I started work on this tonight, but I feel that it would be better to refactor the purge task into a whole process, since this touches the databases as well. @sgdc3 and I would like your input on that. I started permissions work on a new branch that has this issue number in it.

[ljacqu]

Lovely, @Gnat008! From a quick glance, it seems you probably need an additional service class (as you mention) but probably the PurgeTask class needs to stay. It schedules itself with the BukkitScheduler, and we don't want a service class itself to start scheduling itself.

Are you familiar with how the PurgeTask works? The trick behind it is it handles a certain portion of players in an interval, to avoid overloading the server's resources.

[EbonJaeger]

Yeah, I noticed that. Pretty nice, really.

[ljacqu]

Great work @Gnat008, we're one feature richer and you even rid us of some technical debt in the process :) 👍 I'm especially impressed that you took the time to implement the offline permission lookup for each handler!

A few q's about the permissions:

• Could we rename PermissionsManager#hasPermission(String, PermissionNode) to reflect that it's for offline players? It shares the same name with hasPermission(Player, PermissionNode) now, which I find misleading (feel free to disagree).
• For offline player permission lookups, should the default permission be considered? Currently always defaults to false if no handler available. (I don't find it necessary to look up if an offline player is OP, but I'd expect that a permission node with ALLOWED as default will fall back to true.)

The purge process:

• Bit unfortunate we have to duplicate the logic for autorun / regular run (as we did before your refactoring). If you agree I'll look if this can somehow be merged a little; I want to write tests for the PurgeService anyway.

Finally, just a remark: we don't support FlatFile anymore so that logic in there will never come into play. When the DataSource interface changes we typically just implement the method with a throw new UnsupportedOperation()...

[ljacqu]

Oh, one last thing—call me pedantic, but you created PermissionsManager#getHandler to get the permission system type. Would it be possible to remove this and to have a getPermissionSystem() method on PermissionsManager itself? This way we don't expose the entire handler outside of PermissionsManager (no reason anyone needs to communicate with it directly).

It will return null when there is no handler, so in AuthMe:
permsMan.getHandler().getPermissionSystem() == PermissionsSystemType.PERMISSIONS_BUKKIT will have to be changed to PermissionsSystemType.PERMISSIONS_BUKKIT.equals(permsMan.getPermissionSystem()).

[EbonJaeger]

With the permissions, absolutely agreed on both. I just wasn't sure what to name the method otherwise (naming.. heh). Also checking the default permission is a good thing and I probably should have done that.

I'm also unhappy with the duplicate logic for the same reason. If that could be made better, it should.

I implemented the FlatFile thing as well since it implements the interface, and I guess it seemed like a good idea at the time? Probably just made more work for myself doing it, but oh well. lol

[ljacqu]

@Gnat008 I tested the changes and pushed them to master. If you think this issue can be marked as "please verify," please do so. If you no longer need the 674 branch it can be removed. Thanks!

[EbonJaeger]

@rereat :)

[foxi69]

@Gnat008 what is the permission to bypass from purge? :)

[EbonJaeger]

@rereat authme.bypasspurge

[foxi69]

thanks, I am checking it :)

[sgdc3]

@rereat so? ;)

[foxi69]

its just purge the authme database right?
I have 5k essentials userfiles and max 100-200 in use :/
Same in default world, playerdata folder

I wanna purge this folders :)

[sgdc3]

let's give it a try ;)

[ljacqu]

@rereat Could you run it successfully?

[foxi69]

Does not delete the default world's player data :/

[ljacqu]

Could I ask you to open a new issue for this, please? Then we can keep this issue title/number for "implement bypass permission" and we'll have a new issue for "purge does not delete player data." I'm assuming you mean the .dat files of a player are not being deleted?

[sgdc3]

Open a new issue, please ;)