SpaceManiac/PermissionsBukkit — pull request #12 by dawninginnovations (2012-03-29 03:10:55+00:00)
Title: Important Fix to Ensure Application Order of Permissions - Take II

Dear SpaceManiac:

My apologies about the double pull request. I have a habit of running a source formatter to keep code clean. Unfortunately, that has a tendency to mask which lines were actually changed during revision comparison. I have ensured that this is not the case in this second pull request.

There is a problem that has been around since 1.4, which I fixed and pushed to KrinsDeath's, given he was the more current maintainer of a working copy. Attempts to contact you on this matter previously failed. However, he had no interest in accepting it. Hopefully, given the renewed interest in maintaining this plugin, the pull request will find its way into the an official release.

This pull request includes a critical update to ensure permissions are applied in the order in which they are declared rather than an internal order of a HashMap. Regular HashMaps make no guarantee that its elements are arranged according to their insertion order - LinkedHashMaps do.

Furthermore, the order in which permissions are gathered has been changed to reflect the natural order of precedence, rather than a calculated one. Permissions are gathered from the most ancestral group first, overridden by its world-specific settings, working its way down the family tree back to the user. This is **particularly** important when dealing with parent permissions.

Two case studies: 1) Assume the following permission declaration for a user or a group.

mchat.*: false # Ultra paranoid, explicitly turn everything off
mchat.list: true # Turn on /list command
mchat.me: true # Turn on /me command

Given the current implementation, the declaration order is not guaranteed. In real situations, the order can become

mchat.list: true
mchat.me: true
mchat.*: false

as you iterate over a permission collection to add them to a player's permission set. It would not have the expected result. Thus, it is important to use an ordered hash such as LinkedHashMaps to ensure that iteration order is the same as insertion order. This is how SnakeYaml preserves declaration order.

2) Assume the following configuration

users:
Notch:
permissions:
mchat.list: true
groups:
- default

groups:
default:
permissions:
- mchat.*: false

The expected result is that all mchat features are disabled by the group definition, but that its list function is enabled for Notch. However, because the current version populates user permissions first and then collects group permissions the order of application (assuming that they are ordered correctly) is

mchat.list: true
mchat.*: false

instead of

mchat.*: false
mchat.list: true

If it weren't for parent permissions, your current approach would be sufficient. However, even though specific checks are made to see if a permission is already declared before overwriting it, it does not work for parent permissions. Your plugin has no a priori knowledge of what permissions are included in a parent permissions; those are only expanded once they are added to the permissible structures.

**Edit: I just review the stats on this pull, something definitely got hosed in the revision tree. Bear with me as I try to find out what went wrong.**

The only thing I am not certain of is whether I was working with the most current source. Per BukkitDev, PermissionsBukkit is at version 1.6, yet the POM on GitHub shows version 1.5. That is what these modifications are based on. If so, you should still be able to migrate these critical changes over easily. There are additional comments in the source.

Best regards,

Frelling