RestrictSubclaim doesn't work for admin claims

[fallerOfFalls]

What behaviour is observed:

When I try to restrict a subclaim of an admin claim it says "Only an administrator can modify this claim" - even though I have OP and all griefprevention permissions nodes

What behaviour is expected:

a message confirming that the subclaim wouldn't inheret

Steps/models to reproduce:

create an admin claim, then create a subclaim within that admin claim.
stand in the subclaim and do /rsc.

Spigot/Craftbukkit/Paper and GriefPrevention version:

git-Spigot-d21162c-82f3b02 (MC: 1.12.2)
GriefPrevention 16.8

Stack trace/error

This seems like a lot of trouble to go to for something that's so easy to reproduce. If you can't reproduce, then I'll do this.

GriefPrevention config.yml (if applicable)

fresh config.yml (generated by GriefPrevention)

Plugin list (if applicable):

CoreProtect, PermissionsEx, dynmap, WorldEdit, AutismChat4, Gringotts, Vault, VirtualShop, Seen, ConsoleSpamFix, GriefPrevention

[RoboMWM]

You aren't even running a version that contains the /restrictsubclaim command.

[RoboMWM]

15.8 isn't even a version that exists so you might wanna fix that typo.

[RoboMWM]

Anyways, @jacob1

Speaking about this feature, unsure if your PR was the cause but someone's data was corrupted/deleted when someone created a subclaim so yea.... https://www.spigotmc.org/threads/griefprevention.35615/page-237#post-2950103

[jacob1]

It works for me, maybe I can look at the code later and see what kinds of conditions this message shows up in.

Also I kind of doubt this feature caused that bug. They seem to be using the normal file storage, sounds like all the files became empty for some reason. The files are only written when updated I think, so if they all become empty at once it's probably not an issue with this plugin.

[fallerOfFalls]

woops, yeah that was meant to be 16.8, not 15.8. fixed now.

I've no idea what you're talking about with corrupted/deleted data. I've so far not noticed any other issues. Creation of subclaims is fine. and the /RestrictSubclaim command works fine in non-admin subclaims.

[jacob1]

Looks like it's because of this commit I made: a12a3db

My server should be running using that commit ... but maybe it isn't? claim.parent.ownerID is null for admin claims, so that check will always fail.

So either that should be reverted (allow people with permission trust to restrict and unrestrict subclaims), or change it to something like
if ((claim.isAdminClaim() && !player.hasPermission("griefprevention.adminclaims")) ||     !player.getUniqueId().equals(claim.parent.ownerID))
(untested)

[rjohnson98126]

i'd like to confirm this and note that it's rather annoying.

and the default behavior should be to restrict subclaims unless changed otherwise, becuase there is no indication in someones subclaim that they've inherited all the permissions of the parent claim.

for instance, if a user has a shop in a town and one of the admins (accidentally, cluelessly, or maliciously) give that town /containertrust public, not a single subclaim manager will have any idea this has happened until they get looted.

[jacob1]

Having restricted subclaims by default would actually be really annoying. I often work in groups, so whenever I make a new claim I give trust permission to the other person in my group. If I make a subclaim, I want him to automatically have permission too.

I do plan to have a pull request fixing this by the time 1.13 comes out. Then it's up to the plugin owner when to release that.

I will probably use the fix I mentioned, allowing any admin to use /rsc in admin claims, and otherwise still require you to be the claim owner.

[rjohnson98126]

ok sure, i see how one situation would be as common as the other, and it's ultimately a preference.

but here's my bigger point: there is no way for a subclaim manager to see via /trustlist that the parent claim owner has set any particular trusts, so they may have no idea that their entire subclaim has /accesstrust (or worse) automatically granted to some individual, group, or even public.

thats' really unfortunate.

[jacob1]

A subclaim manager is ultimately at the will of whoever created the subclaim for them. It is up to that person to set /restrictsubclaim if it is necessary. /trustlist inside a subclaim will tell you whether it's restricted or not. Perhaps it should also list parent permissions, but that's not as much of an issue.

The only thing I would worry about is if a parent claim manager thinks they are setting permission in a subclaim but set it in the parent instead. But even then, if the subclaims were supposed to be restricted, then /restrictsubclaim should be manually set. On normal survival servers, you usually don't want restricted subclaims by default.

Perhaps it could be a setting (although i'm not going to prioritize that). More settings are always nice.