Skip to content

mf.force.kick and mf.force.rename do not bypass the target faction's role check, unlike mf.force.flag #1988

Description

@dmccoystephenson

Found while documenting the mf.force.* overrides for #1986. Filed rather than fixed, because a documentation cycle should not silently change behavior — and because which way this should go is a design call.

Finding

mf.force.kick and mf.force.rename let an admin name a faction other than their own, but the target faction's own role permission check still runs afterwards against the sender. Since an admin is normally not a member of the faction they are acting on, getRole() returns null and the command is refused — so the force permission does not actually let them force anything.

MfFactionKickCommand.kt — the force permission only selects which faction is resolved:

val faction = if (args.size > 1 && hasForcePermission) {
    factionService.getFaction(args.dropLast(1).joinToString(" "))
} else {
    factionService.getFaction(mfPlayer.id)
}
...
val role = faction.getRole(mfPlayer.id)
if (role == null || !role.hasPermission(faction, plugin.factionPermissions.kick)) {
    sender.sendMessage("$RED${plugin.language["CommandFactionKickNoFactionPermission"]}")
    return@Runnable
}

MfFactionSetNameCommand.kt — same shape:

if (hasForcePermission) {
    val unquotedArgs = args.unquote()
    if (unquotedArgs.size > 1) {
        faction = factionService.getFaction(MfFactionId(unquotedArgs[0])) ?: factionService.getFaction(unquotedArgs[0])
        name = unquotedArgs.drop(1).joinToString(" ")
    }
}
...
val role = faction.getRole(mfPlayer.id)
if (role == null || !role.hasPermission(faction, plugin.factionPermissions.changeName)) {
    player.sendMessage("$RED${plugin.language["CommandFactionSetNameNoFactionPermission"]}")
    return@Runnable
}

Net effect: an op with mf.force.kick who is not in SomeFaction runs /f kick SomeFaction PlayerName and gets "you do not have permission" from the faction role system.

Contrast: mf.force.flag does bypass the role check

MfFactionFlagListCommand.kt / MfFactionFlagSetCommand.kt branch on whether a target faction was given, and skip the role check entirely when one was:

return if (targetFaction != null) {
    if (!sender.hasPermission("mf.force.flag")) { ...deny...; return false }
} else {
    val role = faction.getRole(mfPlayer.id)
    if (role == null || !role.hasPermission(faction, plugin.factionPermissions.setFlag(flag))) { ...deny...; return false }
}

So there are two different interpretations of "force" in the codebase, and plugin.yml's descriptions ("Allows kicking a member from another faction", "Allows forcing faction renames") match the flag interpretation, not the kick/rename one.

Decision needed

  1. Treat kick/rename as the bug — make them skip the faction-role check when a target faction was explicitly named and the force permission is held, matching mf.force.flag and matching the plugin.yml descriptions. This is a permission widening for anyone who already holds these op-default nodes, so it wants a deliberate call rather than a drive-by fix.
  2. Treat the current behavior as intended — the force node only widens targeting, and an admin is expected to combine it with mf.bypass or membership. In that case the plugin.yml descriptions should be reworded.

Interim

The PR for #1986 documents the current behavior accurately in COMMANDS.md, including the caveat that the role check still applies. That wording should be updated if option 1 is taken.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions