#3892 Ticket create and edit should be separate permissions NEEDS MIGRATION

v1.0.0
closed
nobody
Tracker
Cory Johns
2015-08-20
2012-03-14
Chris Tsai
No

Currently, the WRITE permission controls both new ticket creation and editing existing tickets. These should be separated.

Related

Tickets: #3889
Tickets: #3892
Tickets: #4320

Discussion

  • Dave Brondsema

    Dave Brondsema - 2012-05-31
    • labels: support --> support, 42cc
    • milestone: limbo --> forge-backlog
     
  • Dave Brondsema

    Dave Brondsema - 2012-06-01
    • labels: support, 42cc --> support, 42cc, support-migration
     
  • Dave Brondsema

    Dave Brondsema - 2012-06-01
    • labels: support, 42cc, support-migration --> support, 42cc, for-support
     
  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-05

    created #76: [#3892] Ticket create and edit should be separate permissions (2cp)

     

    Related

    Tickets: #3892

  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-07

    closed #76, changes are in 42cc_3892

     
  • Dave Brondsema

    Dave Brondsema - 2012-06-11
    • status: open --> code-review
    • qa: Cory Johns
     
  • Cory Johns

    Cory Johns - 2012-06-12
    • status: code-review --> in-progress
     
  • Cory Johns

    Cory Johns - 2012-06-12

    There needs to be a migration to ensure that all groups that currently have WRITE access now have CREATE and UPDATE, otherwise they will lose the ability to do either.


    Additionally, test cases need to be added or expanded to ensure that CREATE and UPDATE are differentiated properly (e.g., TestFunctionalController.test_render_index needs to check against a group with UPDATE but not CREATE, not just a group with neither, and a test needs to cover the more likely case of having CREATE but not UPDATE, etc).

     
  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-13

    created #80: [#3892] Write migrations and tests (1cp)

     

    Related

    Tickets: #3892

  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-20

    closed #80, all changes are in 42cc_3892 branch

    • status: in-progress --> code-review
     
  • Cory Johns

    Cory Johns - 2012-06-21
    • Iterating over all the app_configs is going to be very expensive. Parameters need to be added to the find() to limit the results from the database (this should eliminate the need for the if a.tool_name == "tickets":, but see below)
    • Unfortunately, our tool_name values are not properly normalized; you need to search case-insensitively (use find({'tool_name': {'$regex': '/^tickets$/i'}})).
    • Loading all 1,457,435 AppConfigs (or even all 54,943 ticket records) in one result set is not ideal. Use utils.chunked_find() (see scripts/migrations/024-migrate-custom-profile-text.py for an example).
    • This will give you the index of the ACL entry you want: i = (i for i,v in enumerate(a.acl) if v.permission == 'write').next() (this should eliminate the need for the while and for loops entirely)
    • You need to get the role_id before deleting the "write" ACL entry
    • There is also the possibility of a "deny write" permission; it's pretty unlikely, but possible, so you should also look for "deny write" and replace it with a pair of "deny create" and "deny update"
     
  • Cory Johns

    Cory Johns - 2012-06-21

    Several of my previous notes apply to the tests as well.


    Additionally, my original main point on the tests was missed. The issue is that a user could configure their tickets system to allow, for example, authenticated users to create tickets but not update them. There are no tests for the situation where a group has CREATE but not UPDATE permissions (nor the other way around, UPDATE but not CREATE, though that's a pretty unlikely case).


    For example, if the group has CREATE permission, but not UPDATE permission, they will see a Create Ticket button on the page, but they will not see an Edit button when viewing a ticket.


    To create such a test, the permissions on the standard test app will need to be adjusted, adding or removing one of CREATE or UPDATE.

     
  • Cory Johns

    Cory Johns - 2012-06-21
    • status: code-review --> open
     
  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-26

    created #91: [#3892] Tests (1cp)

     

    Related

    Tickets: #3892

  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-26
    • status: open --> in-progress
     
  • Yaroslav Luzin

    Yaroslav Luzin - 2012-06-28

    closed #91 and pushed changes into 42cc_3892

    • status: in-progress --> code-review
     
  • Cory Johns

    Cory Johns - 2012-06-28

    Ok, much better. However, I still see two issues:


    test_create_permission could give a false-positive (pass when it shouldn't) if the create fails (the error page won't have the edit button). I think that also asserting that there is no error notify message on the page as well is sufficient. (Could also use response = response.follow(), I think.)


    In the migration script, when you remove an item from a.acl, all the indexes after that will be off by one (since they all shift down due to the removed element). Maybe something like this would work:

    role_ids = [p.role_id for p in a.acl if p.permission == 'write']
    a.acl = [p for p in a.acl if p.permission != 'write']
    for role_id in role_ids:
        a.acl.append(M.ACE.allow(role_id, 'create'))
        a.acl.append(M.ACE.allow(role_id, 'update'))
    
    • status: code-review --> open
     
  • Yaroslav Luzin

    Yaroslav Luzin - 2012-07-02

    created #97: [#3892] Migration script, fix acl stuff (1cp)

    • status: open --> in-progress
     

    Related

    Tickets: #3892

  • Yaroslav Luzin

    Yaroslav Luzin - 2012-07-03

    closed #97, changes are in 42cc_3892

     
  • Dave Brondsema

    Dave Brondsema - 2012-07-05
    • status: in-progress --> code-review
     
  • Cory Johns

    Cory Johns - 2012-07-09
    • status: code-review --> closed
    • size: --> 2
    • milestone: forge-backlog --> forge-jul-13
     
  • Cory Johns

    Cory Johns - 2012-07-09
    • summary: Ticket create and edit should be separate permissions --> Ticket create and edit should be separate permissions NEEDS MIGRATION
    • status: closed --> validation
     
  • Dave Brondsema

    Dave Brondsema - 2012-07-12
    • labels: support, 42cc, for-support --> support, 42cc, for-support, hostedapps
     
  • Dave Brondsema

    Dave Brondsema - 2012-07-12
    • status: validation --> closed
     

Log in to post a comment.