Admin Web UI: Copy permissions from one subject to another

[ethan.jucovy@…]

I often create new Trac environments where I need to restrict access for anonymous users, but want to provide all authenticated users with all the permissions that Trac grants out-of-the-box to anonymous users. This is not easy to do through the admin interface: I need to look at the list of permissions granted to "anonymous"; find each permission one by one in the "Grant Permission" dropdown; and submit the form repeatedly.

Also, I often grant permissions one at a time to specific users of my Trac environment, and only later realize that I want the same permissions to be shared by several users in a group. Transferring those permissions from a user to a group is a similarly time-consuming and error-prone process using the web interface.

A "Copy Permissions" feature would be very helpful for these situations. This could just be a form where the admin fills in a subject (user or group) and a target (user or group). The backend would then find all permissions that are currently granted directly to the subject, and would grant each of those permissions to the target.

[ethan.jucovy@…]

attachment:copy_permissions.patch​ implements this feature and includes a functional test for the behavior. (The test probably ought to be somewhat more thorough — e.g. testing cases where the requesting user does not possess one or more required permissions; cases where the target already possesses some of the subject's permissions; and cases where the subject has no permissions.)

[Ryan J Ollos]

The patch looks good to me. What do you think about?:

• Locating Copy Permissions immediately below Grant Permission
• Adding a trac-admin command. This could be handled in a separate ticket if there isn't time to finish it now, but might be nice for completeness, and your argument about the operations being error-prone seems to hold true for the command line as well.
• The behavior of passing silently when either or both subject and target are empty is consistent with the behavior for the rest of the page, but should we add a warning for those cases (and similarly when adding a Subject to a Group?
• Any idea why the unicode_to_base64 calls are needed in the functional test? It seems like we get away without those elsewhere, but I see the calls used throughout test cases in trac.admin.tests.functional.
• We need to protect against creating subjects with names that are all caps (e.g. TRAC_ADMIN as a subject): All upper-cased tokens are reserved for permission names.

The latest changes can be found in: log:rjollos.git:t11099. Would you like to add the additional functional test cases and address those possible additional changes?

One thing to keep in mind with your patches is to wrap on 79 characters, as described in TracDev/CodingStyle#Miscellaneous.

There might be more issues still. I'll do some additional testing later today or tomorrow.

[Ryan J Ollos]

Replying to rjollos:

• The behavior of passing silently when either or both subject and target are empty is consistent with the behavior for the rest of the page, but should we add a warning for those cases (and similarly when adding a Subject to a Group?

The existing behavior is also consistent with the other admin pages, so I think it would be best to not change it without making consistent changes everywhere.

It looks like we should have functional tests for:

• Normal case of permissions granted.
• Subject doesn't have any permissions.
• Target is all upper-cased tokens.
• Subject has a permission that is no longer defined.
• Actor doesn't possess permission they are trying to grant. Should this be a warning rather than raising a TracError? See screen capture below for behavior.

[Ryan J Ollos]

Functional tests have been added and changes can be found in log:rjollos.git:t11099.2.

Ticket for adding a permission copy command to trac-admin is #11508.

It seems to make more sense to always have the "permission source" on the left, and the "target" on the right, so I've moved the Action select:

[Ryan J Ollos]

A few final changes before committing:

• Replaced string formatting in log message with passing of args in function call.
• Removed markup from warning message for reasons cited in comment:9:ticket:11509.
• Removed some noise from the log message by skipping actions that aren't all upper-case.
• Refactored and extended test cases.

Committed to trunk in [12567:12568].

[Ryan J Ollos]

Thanks Ethan!

[Ryan J Ollos]

Fixed fine-grained permission check in r16334, merged to trunk in r16335.