Apache Allura™

• labels: support --> support, 42cc
• milestone: limbo --> forge-backlog

• labels: support, 42cc --> support, 42cc, support-migration

• labels: support, 42cc, support-migration --> support, 42cc, for-support

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

closed #76, changes are in 42cc_3892

• status: open --> code-review
• qa: Cory Johns

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

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

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

closed #80, all changes are in 42cc_3892 branch

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

• 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"

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.

• status: code-review --> open

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

• status: open --> in-progress

closed #91 and pushed changes into 42cc_3892

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

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

Also, the acl stuff isn't quite right. Suggested changes here: https://sourceforge.net/p/allura/pastebin/4fecfc7ab9363c3bde000000

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

• status: open --> in-progress

closed #97, changes are in 42cc_3892

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

• status: code-review --> closed
• size: --> 2
• milestone: forge-backlog --> forge-jul-13

• summary: Ticket create and edit should be separate permissions --> Ticket create and edit should be separate permissions NEEDS MIGRATION
• status: closed --> validation

• labels: support, 42cc, for-support --> support, 42cc, for-support, hostedapps

• status: validation --> closed