Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

#12226 closed defect (fixed)

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

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2
Component: general Version:
Severity: normal Keywords: permission
Cc: Branch:
Release Notes:

Permission cannot be granted if it differs from a defined action by case only.

API Changes:
Internal Changes:

Description (last modified by 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.

Attachments (0)

Change History (8)

comment:1 by Ryan J Ollos, 8 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 8 years ago

Milestone: next-dev-1.1.x1.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Jun Omae, 8 years ago

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

comment:4 by Ryan J Ollos, 8 years ago

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
Version 0, edited 8 years ago by Ryan J Ollos (next)

comment:5 by Jun Omae, 8 years ago

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:

  • trac/db/tests/api.py

    diff --git a/trac/db/tests/api.py b/trac/db/tests/api.py
    index b2e168324..435d986b3 100644
    a b class ConnectionTestCase(unittest.TestCase):  
    432432        transaction context manager.
    433433        """
    434434        insert_sql = "INSERT INTO blog (bid, author) VALUES (42, 'anonymous')"
    435         with self.env.db_transaction as db:
    436             db(insert_sql)
    437             try:
     435        try:
     436            with self.env.db_transaction as db:
    438437                db(insert_sql)
    439             except self.env.db_exc.IntegrityError:
    440                 pass
     438                db(insert_sql)
     439        except self.env.db_exc.IntegrityError:
     440            pass
    441441
    442442        for _, in self.env.db_query("""
    443443                SELECT author FROM blog WHERE bid=42
    class ConnectionTestCase(unittest.TestCase):  
    449449        inner transaction context manager.
    450450        """
    451451        sql = "INSERT INTO blog (bid, author) VALUES (42, 'anonymous')"
    452         with self.env.db_transaction as db_outer:
    453             db_outer(sql)
    454             with self.env.db_transaction as db_inner:
    455                 try:
     452        try:
     453            with self.env.db_transaction as db_outer:
     454                db_outer(sql)
     455                with self.env.db_transaction as db_inner:
    456456                    db_inner(sql)
    457                 except self.env.db_exc.IntegrityError:
    458                     pass
     457        except self.env.db_exc.IntegrityError:
     458            pass
    459459
    460460        for _, in self.env.db_query("""
    461461                SELECT author FROM blog WHERE bid=42
Last edited 8 years ago by Jun Omae (previous) (diff)

comment:6 by Ryan J Ollos, 8 years ago

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__?

comment:7 by Jun Omae, 8 years ago

The failures would go away by the following patch.

  • trac/perm.py

    diff --git a/trac/perm.py b/trac/perm.py
    index 29a9fe9ef..8961be7d4 100644
    a b class PermissionAdmin(Component):  
    709709                                      "for permission names"))
    710710
    711711        def grant_actions_atomically(actions):
    712             with self.env.db_transaction as db:
    713                 for action in actions:
    714                     try:
    715                         permsys.grant_permission(user, action)
    716                     except PermissionExistsError as e:
    717                         printout(e)
    718                         return action
    719                     except TracError as e:
    720                         raise AdminCommandError(e)
     712            try:
     713                with self.env.db_transaction as db:
     714                    for action in actions:
     715                        permsys.grant_permission(user, action)
     716            except PermissionExistsError as e:
     717                printout(e)
     718                return action
     719            except TracError as e:
     720                raise AdminCommandError(e)
    721721
    722722        # An exception rolls back the atomic transaction so it's
    723723        # necessary to retry the transaction after removing the

Or

  • trac/perm.py

    diff --git a/trac/perm.py b/trac/perm.py
    index 29a9fe9ef..25a52f315 100644
    a b class PermissionAdmin(Component):  
    714714                    try:
    715715                        permsys.grant_permission(user, action)
    716716                    except PermissionExistsError as e:
     717                        db.rollback()
    717718                        printout(e)
    718719                        return action
    719720                    except TracError as e:

in reply to:  description comment:8 by Ryan J Ollos, 8 years ago

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

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.