Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#10957 closed defect (fixed)

User with WIKI_CREATE can't create a wiki page unless they also have WIKI_MODIFY

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos <ryan.j.ollos@…>
Priority: normal Milestone: 1.0.2
Component: wiki system Version: 1.0-stable
Severity: normal Keywords: permissions
Cc: Branch:
Release Notes:

Fixed: Users with WIKI_CREATE permission could not create a wiki page unless they also had WIKI_MODIFY permission.

API Changes:
Internal Changes:

Description

If a user has WIKI_CREATE but not WIKI_MODIFY, any non-existent page will contain text suggesting that a page can be created, but the create page button is not present, so the text is in fact misleading.

User           Action         
------------------------------
anonymous      BROWSER_VIEW   
anonymous      CHANGESET_VIEW 
anonymous      FILE_VIEW      
anonymous      LOG_VIEW       
anonymous      MILESTONE_VIEW 
anonymous      REPORT_SQL_VIEW
anonymous      REPORT_VIEW    
anonymous      ROADMAP_VIEW   
anonymous      SEARCH_VIEW    
anonymous      TICKET_VIEW    
anonymous      TIMELINE_VIEW  
anonymous      WIKI_CREATE    
anonymous      WIKI_VIEW      
authenticated  TICKET_CREATE  
authenticated  TICKET_MODIFY  
authenticated  WIKI_CREATE    
authenticated  WIKI_MODIFY

Second, it is confusing that WIKI_CREATE without WIKI_MODIFY does not actually allow the user to do anything. The user cannot create a page unless they also have WIKI_MODIFY. This could be dealt with in at least two possible ways:

  • WIKI_CREATE granting WIKI_MODIFY.
  • WIKI_CREATE actually allowing pages to be created but not edited after creation.

Is there really a use case for which a user should be able to create pages but not modify them? I tend to think that WIKI_CREATE should grant WIKI_MODIFY.

Patch is forthcoming.

Attachments (1)

WikiCreateOnly.png (31.1 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 11 years ago.

Download all attachments as: .zip

Change History (13)

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: WikiCreateOnly.png added

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

After spending some time on this, I realized that the only issue that needs to be solved here is allowing a user with WIKI_CREATE but not WIKI_MODIFY to actually be able to create a page. With that, the misleading text is no longer misleading, and although I think WIKI_CREATE granting WIKI_MODIFY might be a good idea, such a major change doesn't need to be the subject of this ticket. After applying the patch in 6f05aace, the behavior of WIKI_CREATE mirrors that of TICKET_CREATE, even if having the ability to create but not modify wiki pages doesn't make quite as much sense in a realistic use-case.

I'll follow-up here with some changesets and comments about the functional tests that I've tried to implement for this ticket.

comment:2 by Peter Suter, 11 years ago

Does this change possibly leak information about which (private) pages exist?

in reply to:  2 ; comment:3 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Replying to psuter:

Does this change possibly leak information about which (private) pages exist?

I tried to do some testing with th:PrivateWikiPlugin, but the plugin seems to be completely broken (th:#10634), so I have some work to do there in order to test further with that plugin. What is leading you to think that information might be leaked? The patch shouldn't change the existing behavior for anyone that doesn't have WIKI_CREATE. Fine-grained permission checks are performed before enabling the submit button for users that have WIKI_CREATE.

I have a suspicion of a way in which information could be leaked about the existence of pages, but the issue precedes this patch. I'll do some testing with fine-grained page permissions in place to see if this pans out.

comment:4 by Peter Suter, 11 years ago

Right, I misinterpreted your changes. Sorry for the confusion.

in reply to:  4 comment:5 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Replying to psuter:

Right, I misinterpreted your changes. Sorry for the confusion.

Thanks for taking a look though, I appreciate the review and want to do anything I can to improve the patch so that we can get it integrated.

in reply to:  3 ; comment:6 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Replying to Ryan J Ollos <ryan.j.ollos@…>:

I tried to do some testing with th:PrivateWikiPlugin, but the plugin seems to be completely broken (th:#10634), …

This was my mistake. It mostly works, but has some strange behavior that I hope to improve on in th:#10635.

I've been looking at setting up functional tests for this patch, for which we'd need a user with only TICKET_CREATE and another with only TICKET_MODIFY. The environment that is shared by all of the functional tests doesn't seem to lend itself well to these types of tests. I can see two possibilities for adding the tests:

  • Grant and then remove permissions for the users that have been created when the environment is setup (admin, user, anonymous). Example: logout as user admin (at the start of every test case admin is logged in), grant TICKET_CREATE to anonymous, execute the tests and then cleanup - revoke TICKET_CREATE from anonymous and log back in as admin.
  • Remove the authenticated group when the environment is created, and then create users in the test case, granting them the permissions needed for that test case and give them names that are very unique (e.g. user10957 = "user" + regression ticket #) so that we don't even need to worry about cleaning up these users at the end of the test.

Or maybe I'm overlooking something and there is a much better way?

in reply to:  6 comment:7 by Christian Boos, 11 years ago

Milestone: 1.0.2

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Or maybe I'm overlooking something and there is a much better way?

Well, the way you did it in #10984 seems OK.

comment:8 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Thanks for the feedback. I'll plan to add a functional test to the patch in this ticket within the next week or so (hopefully sooner). #8976 is another that I think just needs a functional test to accompany the minor code change.

comment:9 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Keywords: permissions added

Functional tests have been added to the Git branch.

  • FunctionalTestEnvironment class methods from ticket:11028:t11028-r11682-1.patch were added in 4d83b31a. If ticket:11028:t11028-r11682-1.patch is committed before the changes in this ticket then this changeset can be ignored, with two possible exceptions. I found it necessary to restart the web server even in grant_perm, which will cause a conflict as described in comment:6:ticket:11028 (see traceback and associated comment). Also, documentation for the methods was added.
  • Functional test case added in 994c3117.
  • The following changes were made to the FunctionalTester class:
    • create_wiki_page will now create a random unique CamelCase name for the page if none is specified, and will return the name of the page.
    • Refactored create_wiki_page to make use of the go_to_wiki method.
    • Extracted part of create_wiki_page to a new edit_wiki_page method.
    • create_wiki_page returns to the created wiki page rather than remaining on the timeline page when the method returns. This seems like more correct behavior, though we might have the concern that a plugin could be depending on the existing behavior of remaining on the timeline page.

The changes in the Git branch need to be rebased against the current Trac trunk. I think that what I need to do is rebase the trunk of my Git fork against the current Trac trunk, and then rebase my branch against the trunk. I haven't figured out how to do that yet, but it is on my todo list.

comment:10 by Christian Boos, 11 years ago

Resolution: fixed
Status: newclosed

Fix and test from t10957.2 committed in r11770.

Other refactorings from that branch committed separately in r11772 and r11774 (well, the latter should probably have been part of r11770, but I think I grew tired of shuffling things around ;-) ).

comment:11 by Christian Boos, 11 years ago

Owner: set to Ryan J Ollos <ryan.j.ollos@…>

And thanks again!

comment:12 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Release Notes: modified (diff)

Modify Ticket

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