Fix for messed up ownership protection handling in regards to ally/enemy status

In the process, removed the separate painting handling and made it use the standard block place/destroy checking routine, and otherwise cleaned up the related code a bit

src/com/massivecraft/factions/Faction.java

@@ -11,6 +11,7 @@ import org.bukkit.entity.Player;
 import com.massivecraft.factions.iface.EconomyParticipator;
 import com.massivecraft.factions.iface.RelationParticipator;
 import com.massivecraft.factions.integration.Econ;
+import com.massivecraft.factions.struct.Permission;
 import com.massivecraft.factions.struct.Relation;
 import com.massivecraft.factions.struct.Role;
 import com.massivecraft.factions.util.*;
@@ -631,26 +632,31 @@ public class Faction extends Entity implements EconomyParticipator
 
 	public boolean playerHasOwnershipRights(FPlayer fplayer, FLocation loc)
 	{
-		// sufficient role to bypass ownership?
+		// in own faction, with sufficient role or permission to bypass ownership?
-		if (fplayer.getFaction() == this && fplayer.getRole().isAtLeast(Conf.ownedAreaModeratorsBypass ? Role.MODERATOR : Role.ADMIN))
+		if
+		(
+			fplayer.getFaction() == this
+			&&
+			(
+				fplayer.getRole().isAtLeast(Conf.ownedAreaModeratorsBypass ? Role.MODERATOR : Role.ADMIN)
+				||
+				Permission.OWNERSHIP_BYPASS.has(fplayer.getPlayer())
+			)
+		)
 		{
 			return true;
 		}
 
 		// make sure claimOwnership is initialized
 		if (claimOwnership.isEmpty())
-		{
 			return true;
-		}
 
 		// need to check the ownership list, then
 		Set<String> ownerData = claimOwnership.get(loc);
 
 		// if no owner list, owner list is empty, or player is in owner list, they're allowed
 		if (ownerData == null || ownerData.isEmpty() || ownerData.contains(fplayer.getName().toLowerCase()))
-		{
 			return true;
-		}
 
 		return false;
 	}

src/com/massivecraft/factions/listeners/FactionsBlockListener.java

@@ -127,47 +127,34 @@ public class FactionsBlockListener extends BlockListener
 		Faction otherFaction = Board.getFactionAt(new FLocation(target));
 
 		if (pistonFaction == otherFaction)
-		{
 			return true;
-		}
 
 		if (otherFaction.isNone())
 		{
 			if (!Conf.wildernessDenyBuild || Conf.worldsNoWildernessProtection.contains(target.getWorld().getName()))
-			{
 				return true;
-			}
+
 			return false;
 		}
 		else if (otherFaction.isSafeZone())
 		{
 			if ( ! Conf.safeZoneDenyBuild)
-			{
 				return true;
-			}
+
 			return false;
 		}
 		else if (otherFaction.isWarZone())
 		{
 			if ( ! Conf.warZoneDenyBuild)
-			{
 				return true;
-			}
+
 			return false;
 		}
 
 		Relation rel = pistonFaction.getRelationTo(otherFaction);
-		boolean online = otherFaction.hasPlayersOnline();
 
-		if
+		if (rel.confDenyBuild(otherFaction.hasPlayersOnline()))
-		(
-			(online && (rel.isEnemy() ? Conf.territoryEnemyDenyBuild : (rel.isAlly() ? Conf.territoryAllyDenyBuild : Conf.territoryDenyBuild)))
-			||
-			(!online && (rel.isEnemy() ? Conf.territoryEnemyDenyBuildWhenOffline : (rel.isAlly() ? Conf.territoryAllyDenyBuildWhenOffline : Conf.territoryDenyBuildWhenOffline)))
-		)
-		{
 			return false;
-		}
 
 		return true;
 	}
@@ -177,100 +164,81 @@ public class FactionsBlockListener extends BlockListener
 		FPlayer me = FPlayers.i.get(player);
 
 		if (me.isAdminBypassing())
-		{
 			return true;
-		}
 
 		FLocation loc = new FLocation(location);
 		Faction otherFaction = Board.getFactionAt(loc);
 
-
 		if (otherFaction.isNone())
 		{
 			if (!Conf.wildernessDenyBuild || Conf.worldsNoWildernessProtection.contains(location.getWorld().getName()))
-			{
 				return true; // This is not faction territory. Use whatever you like here.
-			}
+
 			if (!justCheck)
-			{
 				me.sendMessage("You can't "+action+" in the wilderness.");
-			}
+
 			return false;
 		}
 		else if (otherFaction.isSafeZone())
 		{
 			if (!Conf.safeZoneDenyBuild || Permission.MANAGE_SAFE_ZONE.has(player))
-			{
 				return true;
-			}
+
-			if (!justCheck) {
+			if (!justCheck)
 				me.sendMessage("You can't "+action+" in a safe zone.");
-			}
+
 			return false;
 		}
 		else if (otherFaction.isWarZone())
 		{
 			if (!Conf.warZoneDenyBuild || Permission.MANAGE_WAR_ZONE.has(player))
-			{
 				return true;
-			}
+
 			if (!justCheck)
-			{
 				me.sendMessage("You can't "+action+" in a war zone.");
-			}
+
 			return false;
 		}
 
 		Faction myFaction = me.getFaction();
 		Relation rel = myFaction.getRelationTo(otherFaction);
-		boolean ownershipFail = Conf.ownedAreasEnabled && (Conf.ownedAreaDenyBuild || Conf.ownedAreaPainBuild) && !otherFaction.playerHasOwnershipRights(me, loc);
+		boolean online = otherFaction.hasPlayersOnline();
+		boolean pain = !justCheck && rel.confPainBuild(online);
+		boolean deny = rel.confDenyBuild(online);
 
-		// Cancel and/or cause pain (depending on configuration) if we are not in our own territory
+		// hurt the player for building/destroying in other territory?
-		if (!rel.isMember())
+		if (pain)
 		{
-			boolean online = otherFaction.hasPlayersOnline();
+			player.damage(Conf.actionDeniedPainAmount);
-			boolean pain = (!justCheck) && rel.confPainBuild(online);
-			boolean deny = rel.confDenyBuild(online);
 
-			//hurt the player for building/destroying?
+			if (!deny)
-			if (pain)
+				me.sendMessage("It is painful to try to "+action+" in the territory of "+otherFaction.getTag(myFaction));
-			{
-				player.damage(Conf.actionDeniedPainAmount);
-				if (!deny)
-				{
-					me.sendMessage("You are hurt for "+action+" in the territory of "+otherFaction.getTag(myFaction));
-					if (!Conf.ownedAreaDenyBuild)
-					{
-						return true;
-					}
-				}
-			}
-			if (deny)
-			{
-				if (!justCheck)
-				{
-					me.sendMessage("You can't "+action+" in the territory of "+otherFaction.getTag(myFaction));
-				}
-				return false;
-			}
 		}
-		// Also cancel and/or cause pain if player doesn't have ownership rights for this claim
+
-		else if (rel.isMember() && ownershipFail && ! Permission.OWNERSHIP_BYPASS.has(player))
+		// cancel building/destroying in other territory?
+		if (deny)
 		{
-			if (Conf.ownedAreaPainBuild && !justCheck)
+			if (!justCheck)
+				me.sendMessage("You can't "+action+" in the territory of "+otherFaction.getTag(myFaction));
+
+			return false;
+ 		}
+
+		// Also cancel and/or cause pain if player doesn't have ownership rights for this claim
+		if (Conf.ownedAreasEnabled && (Conf.ownedAreaDenyBuild || Conf.ownedAreaPainBuild) && !otherFaction.playerHasOwnershipRights(me, loc))
+		{
+			if (!pain && Conf.ownedAreaPainBuild && !justCheck)
 			{
 				player.damage(Conf.actionDeniedPainAmount);
+
 				if (!Conf.ownedAreaDenyBuild)
-				{
+					me.sendMessage("It is painful to try to "+action+" in this territory, it is owned by: "+otherFaction.getOwnerListString(loc));
-					me.sendMessage("You are hurt for "+action+" in this territory, it is owned by: "+myFaction.getOwnerListString(loc));
-				}
 			}
 			if (Conf.ownedAreaDenyBuild)
 			{
 				if (!justCheck)
-				{
+					me.sendMessage("You can't "+action+" in this territory, it is owned by: "+otherFaction.getOwnerListString(loc));
-					me.sendMessage("You can't "+action+" in this territory, it is owned by: "+myFaction.getOwnerListString(loc));
+
-				}
 				return false;
 			}
 		}

src/com/massivecraft/factions/listeners/FactionsEntityListener.java

@@ -117,14 +117,17 @@ public class FactionsEntityListener extends EntityListener
 		Location loc = event.getLocation();
 		
 		Faction faction = Board.getFactionAt(new FLocation(loc));
-		boolean online = faction.hasPlayersOnline();
 
 		if (faction.noExplosionsInTerritory())
 		{
 			// faction is peaceful and has explosions set to disabled
 			event.setCancelled(true);
+			return;
 		}
-		else if
+
+		boolean online = faction.hasPlayersOnline();
+
+		if
 		(
 			event.getEntity() instanceof Creeper
 			&&
@@ -418,9 +421,7 @@ public class FactionsEntityListener extends EntityListener
 			return;
 		}
 
-		FLocation loc = new FLocation(event.getPainting().getLocation());
+		if ( ! FactionsBlockListener.playerCanBuildDestroyBlock((Player)breaker, event.getPainting().getLocation(), "remove paintings", false))
-
-		if ( ! this.playerCanDoPaintings((Player)breaker, loc, "remove"))
 		{
 			event.setCancelled(true);
 		}
@@ -431,71 +432,12 @@ public class FactionsEntityListener extends EntityListener
 	{
 		if (event.isCancelled()) return;
 
-		if ( ! this.playerCanDoPaintings(event.getPlayer(), new FLocation(event.getBlock()), "place"))
+		if ( ! FactionsBlockListener.playerCanBuildDestroyBlock(event.getPlayer(), event.getBlock().getLocation(), "place paintings", false) )
 		{
 			event.setCancelled(true);
 		}
 	}
 
-	public boolean playerCanDoPaintings(Player player, FLocation loc, String action)
-	{
-		FPlayer me = FPlayers.i.get(player);
-		if (me.isAdminBypassing())
-		{
-			return true;
-		}
-
-		Faction otherFaction = Board.getFactionAt(loc);
-
-		if (otherFaction.isNone())
-		{
-			if (!Conf.wildernessDenyBuild || Conf.worldsNoWildernessProtection.contains(player.getWorld().getName()))
-			{
-				return true; // This is not faction territory. Use whatever you like here.
-			}
-			me.sendMessage("You can't "+action+" paintings in the wilderness.");
-			return false;
-		}
-
-		if (otherFaction.isSafeZone())
-		{
-			if (Permission.MANAGE_SAFE_ZONE.has(player) || !Conf.safeZoneDenyBuild)
-			{
-				return true;
-			}
-			me.sendMessage("You can't "+action+" paintings in a safe zone.");
-			return false;
-		}
-		else if (otherFaction.isWarZone())
-		{
-			if (Permission.MANAGE_WAR_ZONE.has(player) || !Conf.warZoneDenyBuild)
-			{
-				return true;
-			}
-			me.sendMessage("You can't "+action+" paintings in a war zone.");
-			return false;
-		}
-
-		Faction myFaction = me.getFaction();
-		Relation rel = myFaction.getRelationTo(otherFaction);
-		boolean ownershipFail = Conf.ownedAreasEnabled && Conf.ownedAreaDenyBuild && !otherFaction.playerHasOwnershipRights(me, loc);
-
-		// Cancel if we are not in our own territory and building should be denied
-		if (!rel.isMember() && rel.confDenyBuild(otherFaction.hasPlayersOnline()))
-		{
-			me.sendMessage("You can't "+action+" paintings in the territory of "+otherFaction.getTag(myFaction));
-			return false;
-		}
-		// Also cancel if player doesn't have ownership rights for this claim
-		else if (rel.isMember() && ownershipFail && !Permission.OWNERSHIP_BYPASS.has(player))
-		{
-			me.sendMessage("You can't "+action+" paintings in this territory, it is owned by: "+otherFaction.getOwnerListString(loc));
-			return false;
-		}
-
-		return true;
-	}
-
 	@Override
 	public void onEndermanPickup(EndermanPickupEvent event)
 	{

src/com/massivecraft/factions/listeners/FactionsPlayerListener.java

@@ -231,6 +231,9 @@ public class FactionsPlayerListener extends PlayerListener
 			Faction myFaction = me.getFaction();
 			// TODO: Why is this ("cost") here and unused??? Should it be used somewhere Brettflan? :)
 			// Olof just commented it out to avoid the error.
+			// So sayeth Brettflan, answered in a comment since you asked in a comment:
+			// NOTHING MORE TODO: it was used, but apparently not needed after some changes including this commit: https://github.com/MassiveCraft/Factions/commit/5eaf9c68358c6076bb856baf80fd6496e2ab02ce
+			//
 			//Faction otherFaction = Board.getFactionAt(to);
 			//double cost = Econ.calculateClaimCost(myFaction.getLandRounded(), otherFaction.isNormal());
 
@@ -323,87 +326,71 @@ public class FactionsPlayerListener extends PlayerListener
 	{
 		FPlayer me = FPlayers.i.get(player);
 		if (me.isAdminBypassing())
-		{
 			return true;
-		}
 
 		FLocation loc = new FLocation(location);
 		Faction otherFaction = Board.getFactionAt(loc);
 
-		if (otherFaction.hasPlayersOnline()){
+		if (otherFaction.hasPlayersOnline())
+		{
 			if ( ! Conf.territoryDenyUseageMaterials.contains(material))
-			{
 				return true; // Item isn't one we're preventing for online factions.
-			}
 		}
 		else
 		{
 			if ( ! Conf.territoryDenyUseageMaterialsWhenOffline.contains(material))
-			{
 				return true; // Item isn't one we're preventing for offline factions.
-			}
 		}
 
-		
-
 		if (otherFaction.isNone())
 		{
 			if (!Conf.wildernessDenyUseage || Conf.worldsNoWildernessProtection.contains(location.getWorld().getName()))
-			{
 				return true; // This is not faction territory. Use whatever you like here.
-			}
 			
 			if (!justCheck)
-			{
 				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in the wilderness.");
-			}
+
 			return false;
 		}
 		else if (otherFaction.isSafeZone())
 		{
 			if (!Conf.safeZoneDenyUseage || Permission.MANAGE_SAFE_ZONE.has(player))
-			{
 				return true;
-			}
+
 			if (!justCheck)
-			{
 				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in a safe zone.");
-			}
+
 			return false;
 		}
 		else if (otherFaction.isWarZone())
 		{
 			if (!Conf.warZoneDenyUseage || Permission.MANAGE_WAR_ZONE.has(player))
-			{
 				return true;
-			}
+
 			if (!justCheck)
-			{
 				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in a war zone.");
-			}
+
 			return false;
 		}
 
 		Faction myFaction = me.getFaction();
 		Relation rel = myFaction.getRelationTo(otherFaction);
-		boolean ownershipFail = Conf.ownedAreasEnabled && Conf.ownedAreaDenyUseage && !otherFaction.playerHasOwnershipRights(me, loc);
 
 		// Cancel if we are not in our own territory
-		if (!rel.isMember() && rel.confDenyUseage())
+		if (rel.confDenyUseage())
 		{
 			if (!justCheck)
-			{
 				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in the territory of "+otherFaction.getTag(myFaction));
-			}
+
 			return false;
 		}
+
 		// Also cancel if player doesn't have ownership rights for this claim
-		else if (rel.isMember() && ownershipFail && ! Permission.OWNERSHIP_BYPASS.has(player))
+		if (Conf.ownedAreasEnabled && Conf.ownedAreaDenyUseage && !otherFaction.playerHasOwnershipRights(me, loc))
 		{
 			if (!justCheck)
-			{
+				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in this territory, it is owned by: "+otherFaction.getOwnerListString(loc));
-				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in this territory, it is owned by: "+myFaction.getOwnerListString(loc));
+
-			}
 			return false;
 		}
 
@@ -414,9 +401,7 @@ public class FactionsPlayerListener extends PlayerListener
 	{
 		FPlayer me = FPlayers.i.get(player);
 		if (me.isAdminBypassing())
-		{
 			return true;
-		}
 
 		Material material = block.getType();
 		FLocation loc = new FLocation(block);
@@ -424,47 +409,38 @@ public class FactionsPlayerListener extends PlayerListener
 
 		// no door/chest/whatever protection in wilderness, war zones, or safe zones
 		if (!otherFaction.isNormal())
-		{
 			return true;
-		}
 
 		// We only care about some material types.
 		if (otherFaction.hasPlayersOnline())
 		{
 			if ( ! Conf.territoryProtectedMaterials.contains(material))
-			{
 				return true;
-			}
 		}
 		else
 		{
 			if ( ! Conf.territoryProtectedMaterialsWhenOffline.contains(material))
-			{
 				return true;
-			}
 		}
 
-		
 		Faction myFaction = me.getFaction();
 		Relation rel = myFaction.getRelationTo(otherFaction);
-		boolean ownershipFail = Conf.ownedAreasEnabled && Conf.ownedAreaProtectMaterials && !otherFaction.playerHasOwnershipRights(me, loc);
 
 		// You may use any block unless it is another faction's territory...
 		if (rel.isNeutral() || (rel.isEnemy() && Conf.territoryEnemyProtectMaterials) || (rel.isAlly() && Conf.territoryAllyProtectMaterials))
 		{
 			if (!justCheck)
-			{
 				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in the territory of "+otherFaction.getTag(myFaction));
-			}
+
 			return false;
 		}
+
 		// Also cancel if player doesn't have ownership rights for this claim
-		else if (rel.isMember() && ownershipFail && ! Permission.OWNERSHIP_BYPASS.has(player))
+		if (Conf.ownedAreasEnabled && Conf.ownedAreaProtectMaterials && !otherFaction.playerHasOwnershipRights(me, loc))
 		{
 			if (!justCheck)
-			{
+				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in this territory, it is owned by: "+otherFaction.getOwnerListString(loc));
-				me.sendMessage("You can't use "+TextUtil.getMaterialName(material)+" in this territory, it is owned by: "+myFaction.getOwnerListString(loc));
+
-			}
 			return false;
 		}