Prevent granting permission that differs from a defined action by case only

[Ryan J Ollos]

The following issue was reported in ​gmessage:trac-users:vKI_MDGL-Fs/ecyzUJLqBAAJ. The user had granted TRAC_Admin to a user, and when attempting to grant TRAC_ADMIN, the system replied that the user already had TRAC_ADMIN. I haven't tried to reproduce yet with MySQL.

It would seem that the most straightforward way to avoid confusion would be to prevent users from adding a permissions group that differs from a defined action only by case. Further, to handle the scenario in which a component is enabled after a permission is granted we should provide a more accurate error message, showing the actual casing of the permission that has already been granted.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12226_permission_casing.

[Jun Omae]

I got 2 failures with MySQL. Unit and functional tests pass with SQLite and PostgreSQL.

======================================================================
FAIL: test_permission_add_already_exists (trac.admin.tests.console.TracadminTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 364, in test_permission_add_already_exists
    self.assertExpectedResult(output + output2)
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 133, in assertExpectedResult
    self.assertEqual(expected_result, output)
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 156, in assertEqual
    output, diff()))
AssertionError: u'The user anonymous already has permission WIKI_VIEW.\n\nUser           Action\n------------------------------\nanonymous      BROWSER_VIEW\nanonymous      CHANGESET_VIEW\nanonymous      FILE_VIEW\nanonymous      LOG_VIEW\nanonymous      MILESTONE_VIEW\nanonymous      REPORT_SQL_VIEW\nanonymous      REPORT_VIEW\nanonymous      ROADMAP_VIEW\nanonymous      SEARCH_VIEW\nanonymous      TICKET_VIEW\nanonymous      TIMELINE_VIEW\nanonymous      WIKI_CREATE\nanonymous      WIKI_MODIFY\nanonymous      WIKI_VIEW\nauthenticated  TICKET_CREATE\nauthenticated  TICKET_MODIFY\nauthenticated  WIKI_CREATE\nauthenticated  WIKI_MODIFY\n\n\nAvailable actions:\n BROWSER_VIEW, CHANGESET_VIEW, CONFIG_VIEW, EMAIL_VIEW, FILE_VIEW,\n LOG_VIEW, MILESTONE_ADMIN, MILESTONE_CREATE, MILESTONE_DELETE,\n MILESTONE_MODIFY, MILESTONE_VIEW, PERMISSION_ADMIN, PERMISSION_GRANT,\n PERMISSION_REVOKE, REPORT_ADMIN, REPORT_CREATE, REPORT_DELETE,\n REPORT_MODIFY, REPORT_SQL_VIEW, REPORT_VIEW, ROADMAP_ADMIN, ROADMAP_VIEW,\n SEARCH_VIEW, TICKET_ADMIN, TICKET_APPEND, TICKET_BATCH_MODIFY,\n TICKET_CHGPROP, TICKET_CREATE, TICKET_EDIT_CC, TICKET_EDIT_COMMENT,\n TICKET_EDIT_DESCRIPTION, TICKET_MODIFY, TICKET_VIEW, TIMELINE_VIEW,\n TRAC_ADMIN, VERSIONCONTROL_ADMIN, WIKI_ADMIN, WIKI_CREATE, WIKI_DELETE,\n WIKI_MODIFY, WIKI_RENAME, WIKI_VIEW\n\n' != u'The user anonymous already has permission WIKI_VIEW.\nThe user anonymous already has permission WIKI_CREATE.\n\nUser           Action\n------------------------------\nanonymous      BROWSER_VIEW\nanonymous      CHANGESET_VIEW\nanonymous      FILE_VIEW\nanonymous      LOG_VIEW\nanonymous      MILESTONE_VIEW\nanonymous      REPORT_SQL_VIEW\nanonymous      REPORT_VIEW\nanonymous      ROADMAP_VIEW\nanonymous      SEARCH_VIEW\nanonymous      TICKET_VIEW\nanonymous      TIMELINE_VIEW\nanonymous      WIKI_CREATE\nanonymous      WIKI_MODIFY\nanonymous      WIKI_VIEW\nauthenticated  TICKET_CREATE\nauthenticated  TICKET_MODIFY\nauthenticated  WIKI_CREATE\nauthenticated  WIKI_MODIFY\n\n\nAvailable actions:\n BROWSER_VIEW, CHANGESET_VIEW, CONFIG_VIEW, EMAIL_VIEW, FILE_VIEW,\n LOG_VIEW, MILESTONE_ADMIN, MILESTONE_CREATE, MILESTONE_DELETE,\n MILESTONE_MODIFY, MILESTONE_VIEW, PERMISSION_ADMIN, PERMISSION_GRANT,\n PERMISSION_REVOKE, REPORT_ADMIN, REPORT_CREATE, REPORT_DELETE,\n REPORT_MODIFY, REPORT_SQL_VIEW, REPORT_VIEW, ROADMAP_ADMIN, ROADMAP_VIEW,\n SEARCH_VIEW, TICKET_ADMIN, TICKET_APPEND, TICKET_BATCH_MODIFY,\n TICKET_CHGPROP, TICKET_CREATE, TICKET_EDIT_CC, TICKET_EDIT_COMMENT,\n TICKET_EDIT_DESCRIPTION, TICKET_MODIFY, TICKET_VIEW, TIMELINE_VIEW,\n TRAC_ADMIN, VERSIONCONTROL_ADMIN, WIKI_ADMIN, WIKI_CREATE, WIKI_DELETE,\n WIKI_MODIFY, WIKI_RENAME, WIKI_VIEW\n\n'
--- expected
+++ actual
@@ -1,4 +1,5 @@
 The user anonymous already has permission WIKI_VIEW.
+The user anonymous already has permission WIKI_CREATE.
 
 User           Action
 ------------------------------


======================================================================
FAIL: test_permission_add_subject_already_in_group (trac.admin.tests.console.TracadminTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 379, in test_permission_add_subject_already_in_group
    self.assertExpectedResult(output2 + output3)
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 133, in assertExpectedResult
    self.assertEqual(expected_result, output)
  File "/run/shm/24a1ec431c1cf9785320515e7e14312d45b29af8/py27-mysql/trac/admin/tests/console.py", line 156, in assertEqual
    output, diff()))
AssertionError: u'The user user1 is already in the group group2.\n\nUser           Action\n------------------------------\nanonymous      BROWSER_VIEW\nanonymous      CHANGESET_VIEW\nanonymous      FILE_VIEW\nanonymous      LOG_VIEW\nanonymous      MILESTONE_VIEW\nanonymous      REPORT_SQL_VIEW\nanonymous      REPORT_VIEW\nanonymous      ROADMAP_VIEW\nanonymous      SEARCH_VIEW\nanonymous      TICKET_VIEW\nanonymous      TIMELINE_VIEW\nanonymous      WIKI_VIEW\nauthenticated  TICKET_CREATE\nauthenticated  TICKET_MODIFY\nauthenticated  WIKI_CREATE\nauthenticated  WIKI_MODIFY\nuser1          group1\nuser1          group2\nuser1          group3\n\n\nAvailable actions:\n BROWSER_VIEW, CHANGESET_VIEW, CONFIG_VIEW, EMAIL_VIEW, FILE_VIEW,\n LOG_VIEW, MILESTONE_ADMIN, MILESTONE_CREATE, MILESTONE_DELETE,\n MILESTONE_MODIFY, MILESTONE_VIEW, PERMISSION_ADMIN, PERMISSION_GRANT,\n PERMISSION_REVOKE, REPORT_ADMIN, REPORT_CREATE, REPORT_DELETE,\n REPORT_MODIFY, REPORT_SQL_VIEW, REPORT_VIEW, ROADMAP_ADMIN, ROADMAP_VIEW,\n SEARCH_VIEW, TICKET_ADMIN, TICKET_APPEND, TICKET_BATCH_MODIFY,\n TICKET_CHGPROP, TICKET_CREATE, TICKET_EDIT_CC, TICKET_EDIT_COMMENT,\n TICKET_EDIT_DESCRIPTION, TICKET_MODIFY, TICKET_VIEW, TIMELINE_VIEW,\n TRAC_ADMIN, VERSIONCONTROL_ADMIN, WIKI_ADMIN, WIKI_CREATE, WIKI_DELETE,\n WIKI_MODIFY, WIKI_RENAME, WIKI_VIEW\n\n' != u'The user user1 is already in the group group2.\nThe user user1 is already in the group group1.\n\nUser           Action\n------------------------------\nanonymous      BROWSER_VIEW\nanonymous      CHANGESET_VIEW\nanonymous      FILE_VIEW\nanonymous      LOG_VIEW\nanonymous      MILESTONE_VIEW\nanonymous      REPORT_SQL_VIEW\nanonymous      REPORT_VIEW\nanonymous      ROADMAP_VIEW\nanonymous      SEARCH_VIEW\nanonymous      TICKET_VIEW\nanonymous      TIMELINE_VIEW\nanonymous      WIKI_VIEW\nauthenticated  TICKET_CREATE\nauthenticated  TICKET_MODIFY\nauthenticated  WIKI_CREATE\nauthenticated  WIKI_MODIFY\nuser1          group1\nuser1          group2\nuser1          group3\n\n\nAvailable actions:\n BROWSER_VIEW, CHANGESET_VIEW, CONFIG_VIEW, EMAIL_VIEW, FILE_VIEW,\n LOG_VIEW, MILESTONE_ADMIN, MILESTONE_CREATE, MILESTONE_DELETE,\n MILESTONE_MODIFY, MILESTONE_VIEW, PERMISSION_ADMIN, PERMISSION_GRANT,\n PERMISSION_REVOKE, REPORT_ADMIN, REPORT_CREATE, REPORT_DELETE,\n REPORT_MODIFY, REPORT_SQL_VIEW, REPORT_VIEW, ROADMAP_ADMIN, ROADMAP_VIEW,\n SEARCH_VIEW, TICKET_ADMIN, TICKET_APPEND, TICKET_BATCH_MODIFY,\n TICKET_CHGPROP, TICKET_CREATE, TICKET_EDIT_CC, TICKET_EDIT_COMMENT,\n TICKET_EDIT_DESCRIPTION, TICKET_MODIFY, TICKET_VIEW, TIMELINE_VIEW,\n TRAC_ADMIN, VERSIONCONTROL_ADMIN, WIKI_ADMIN, WIKI_CREATE, WIKI_DELETE,\n WIKI_MODIFY, WIKI_RENAME, WIKI_VIEW\n\n'
--- expected
+++ actual
@@ -1,4 +1,5 @@
 The user user1 is already in the group group2.
+The user user1 is already in the group group1.
 
 User           Action
 ------------------------------


----------------------------------------------------------------------
Ran 2015 tests in 615.641s

FAILED (failures=2)
make: *** [unit-test] Error 1

[Ryan J Ollos]

It looks like the test failures are due to the database transaction not being rolled back when there is an exception within the scope of a transaction context manager. The issue is only seen with MySQL. I've added a test case to demonstrate the issue: [b28cdbce/rjollos.git].

python setup.py -q test -s trac.db.tests.api.suite 
..........................FF.................
======================================================================
FAIL: test_rollback_nested_transaction_on_exception (trac.db.tests.api.ConnectionTestCase)
Transaction is rolled back when an exception occurs in the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/tests/api.py", line 463, in test_rollback_nested_transaction_on_exception
    self.fail("Transaction was not rolled back")
AssertionError: Transaction was not rolled back

======================================================================
FAIL: test_rollback_transaction_on_exception (trac.db.tests.api.ConnectionTestCase)
Transaction is rolled back when an exception occurs in the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/db/tests/api.py", line 445, in test_rollback_transaction_on_exception
    self.fail("Transaction was not rolled back")
AssertionError: Transaction was not rolled back

----------------------------------------------------------------------
Ran 45 tests in 0.600s

FAILED (failures=2)
make: *** [all] Error 1

[Jun Omae]

For MySQL, a failure of query wouldn't lead abort of the transaction, e.g not found table and unique key violation. We should catch exceptions outside a transaction context manager. Otherwise, explicitly call db.rollback().

See also:

• tags/trac-1.1.6/trac/web/session.py@:130,132-133#L116​
• tags/trac-1.1.6/trac/versioncontrol/cache.py@:183,191-193#L162​

trac/db/tests/api.py

@@ -432 +432 @@
         transaction context manager.
         """
         insert_sql = "INSERT INTO blog (bid, author) VALUES (42, 'anonymous')"
-        with self.env.db_transaction as db:
-            db(insert_sql)
-            try:
+        try:
+            with self.env.db_transaction as db:
                 db(insert_sql)
-            except self.env.db_exc.IntegrityError:
-                pass
+                db(insert_sql)
+        except self.env.db_exc.IntegrityError:
+            pass
 
         for _, in self.env.db_query("""
                 SELECT author FROM blog WHERE bid=42
@@ -449 +449 @@
         inner transaction context manager.
         """
         sql = "INSERT INTO blog (bid, author) VALUES (42, 'anonymous')"
-        with self.env.db_transaction as db_outer:
-            db_outer(sql)
-            with self.env.db_transaction as db_inner:
-                try:
+        try:
+            with self.env.db_transaction as db_outer:
+                db_outer(sql)
+                with self.env.db_transaction as db_inner:
                     db_inner(sql)
-                except self.env.db_exc.IntegrityError:
-                    pass
+        except self.env.db_exc.IntegrityError:
+            pass
 
         for _, in self.env.db_query("""
                 SELECT author FROM blog WHERE bid=42

[Ryan J Ollos]

I was expecting that the transaction context manager would roll back the transaction on any exception: tags/trac-1.1.6/trac/db/api.py@:136-137,157#L132​. Is the issue that we'd need to let the exception propagate up the stack for it to be handled in __exit__?

[Jun Omae]

The failures would go away by the following patch.

trac/perm.py

@@ -709 +709 @@
                                       "for permission names"))
 
         def grant_actions_atomically(actions):
-            with self.env.db_transaction as db:
-                for action in actions:
-                    try:
-                        permsys.grant_permission(user, action)
-                    except PermissionExistsError as e:
-                        printout(e)
-                        return action
-                    except TracError as e:
-                        raise AdminCommandError(e)
+            try:
+                with self.env.db_transaction as db:
+                    for action in actions:
+                        permsys.grant_permission(user, action)
+            except PermissionExistsError as e:
+                printout(e)
+                return action
+            except TracError as e:
+                raise AdminCommandError(e)
 
         # An exception rolls back the atomic transaction so it's
         # necessary to retry the transaction after removing the

Or

trac/perm.py

@@ -714 +714 @@
                     try:
                         permsys.grant_permission(user, action)
                     except PermissionExistsError as e:
+                        db.rollback()
                         printout(e)
                         return action
                     except TracError as e:

[Ryan J Ollos]

Replying to Ryan J Ollos:

It would seem that the most straightforward way to avoid confusion would be to prevent users from adding a permissions group that differs from a defined action only by case.

Fixed in [14359].

Further, to handle the scenario in which a component is enabled after a permission is granted we should provide a more accurate error message, showing the actual casing of the permission that has already been granted.

I haven't found a way to implement a fix that is not overly complex. It's an edge case so I think we are okay to leave the existing behavior.

Other issues discussed in this ticket will be addressed in #12242.