Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 15 months ago

#11099 closed enhancement (fixed)

Admin Web UI: Copy permissions from one subject to another

Reported by: ethan.jucovy@… Owned by: ethan.jucovy@…
Priority: normal Milestone: 1.1.2
Component: admin/web Version:
Severity: normal Keywords: permissions
Cc: leho@…, olemis+trac@…
Release Notes:

Added a Copy Permissions form to the Permissions admin page.

API Changes:

Description

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.

Attachments (4)

copy_permissions.patch (4.7 KB ) - added by ethan.jucovy@… 6 years ago.
ManagePermissionsAndGroups.png (42.6 KB ) - added by Ryan J Ollos 5 years ago.
ActorDoesntHavePermission.png (68.0 KB ) - added by Ryan J Ollos 5 years ago.
ActionOnLeft.png (25.6 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by ethan.jucovy@…

Attachment: copy_permissions.patch added

comment:1 Changed 6 years ago by 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.)

comment:2 Changed 6 years ago by lkraav <leho@…>

Cc: leho@… added

comment:3 Changed 6 years ago by Christian Boos

Keywords: permissions added
Milestone: next-dev-1.1.x
Owner: set to Christian Boos
Status: newassigned

comment:4 Changed 5 years ago by 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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:5 Changed 5 years ago by Olemis Lang <olemis+trac@…>

Cc: olemis+trac@… added

Changed 5 years ago by Ryan J Ollos

comment:6 in reply to:  4 Changed 5 years ago by 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.

Changed 5 years ago by Ryan J Ollos

comment:7 Changed 5 years ago by Ryan J Ollos

Milestone: next-dev-1.1.x1.1.3
Owner: changed from Christian Boos to Ryan J Ollos

Changed 5 years ago by Ryan J Ollos

Attachment: ActionOnLeft.png added

comment:8 Changed 5 years ago by 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:

comment:9 Changed 5 years ago by Ryan J Ollos

Milestone: 1.1.31.1.2
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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].

comment:10 Changed 5 years ago by Ryan J Ollos

Owner: changed from Ryan J Ollos to ethan.jucovy@…

Thanks Ethan!

comment:11 Changed 15 months ago by Ryan J Ollos

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain ethan.jucovy@….
The resolution will be deleted.
to The owner will be changed from ethan.jucovy@… to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.