Permissions: setting a Wiki page "read-only" does not restrict adding attachments

[daira@…]

Observed behaviour:

I edit a Wiki page and set it to "read-only". As expected, only users with the TRAC_ADMIN permission can edit it or set it back to read/write. However, there is no change in the permission required to add attachments (ATTACHMENT_CREATE) for that page.

Expected behaviour:

Either TRAC_ADMIN is required to add attachments to a read-only page, or there is some other permission distinct from ATTACHMENT_CREATE that is needed to add attachments to read-only pages. Other pages continue to require only ATTACHMENT_CREATE.

Rationale: this is needed to discourage spam on high-visibility pages.

[Ryan J Ollos]

Replying to daira@…:

Observed behaviour:

I edit a Wiki page and set it to "read-only". As expected, only users with the TRAC_ADMIN permission can edit it or set it back to read/write.

However, there is no change in the permission required to add attachments (ATTACHMENT_CREATE) for that page.

I investigated your report and found that when a page is read-only, the Attach file button is no longer present on the wiki page, however the user can directly navigate to /attachment/wiki/PageName and add an attachment provided that they have WIKI_MODIFY.

Expected behaviour:

Either TRAC_ADMIN is required to add attachments to a read-only page, or there is some other permission distinct from ATTACHMENT_CREATE that is needed to add attachments to read-only pages. Other pages continue to require only ATTACHMENT_CREATE.

Rationale: this is needed to discourage spam on high-visibility pages.

I'm not sure of the best way to fix this. We could deny permission in LegacyAttachmentPolicy.check_permission, but on the other hand, it doesn't seem like that is the right place to be doing realm-specific permissions checks. Could the readonly attribute be generalized and added to the Resource object? Since that's not presently the case, it seems like the only way to fix this is to add some special behavior for wiki pages in the attachment module.

That, or maybe a fresh look at this on another day might reveal a better solution.

If you need a workaround, I think you could probably prevent attachments to a set of wiki pages using TracFineGrainedPermissions. I haven't tested to be sure that it doesn't have a similar defect for attachments though.

[Peter Suter]

Would it make sense to fix this not only for attachments, but to generally deny WIKI_MODIFY for readonly pages? A new Wiki specific permission policy would be needed. I.e. something like:

class WikiModule(Component):
    implements(..., IPermissionPolicy)

    ...
    
    def check_permission(self, action, username, resource, perm):
        if resource and resource.realm == 'wiki' and action == 'WIKI_MODIFY':
            page = WikiPage(self.env, resource.id)
            if page.readonly:
                return 'WIKI_ADMIN' in perm

I have no tested if this works. Just fixing it for attachments in LegacyAttachmentPolicy might be simpler.

[Ryan J Ollos]

That's an interesting idea. I took a crack at implementing it in repos:rjollos.git:t11244. The branch was tested with permission_policies = WikiPermissionPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy.

I like that it localizes all of the elevated permission checks for read-only pages to a single location. Additional work is needed - adding functional tests and an environment upgrade step.

Removing WikiPermissionPolicy from permission_policies in trac.ini would result in a confusing situation in which there is a read-only checkbox on the form that has no effect. Therefore we should probably not add the read-only checkbox to the edit form unless WikiPermissionPolicy is active.

[Peter Suter]

Nice, I like it. At least if it works robustly.

Replying to rjollos:

permission_policies = WikiPermissionPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy.

The ordering of permission_policies can be critical. Does it matter here, or should this work in any order?

Maybe instead of return 'WIKI_ADMIN' in perm we should only if not 'WIKI_ADMIN' in perm: return False, so other policies can deny permissions for other reasons.

Another critical aspect is the perm check. Can there be any non-obvious behavior triggered by recursion?

It seems to me there should be no problems, at least with the default policies. But tests would for sure be appropriate, even if they can't prove the absence of problems with third-party policies.

You mention an environment upgrade step, I assume to automatically enable this policy. Is there a precedent for doing something like that? AuthzSourcePolicy was not added automatically. This case may be different of course. It seems safer to do so, but maybe there are scenarios with other policies where it's not?

What do you think?

[Ryan J Ollos]

Replying to psuter:

Nice, I like it. At least if it works robustly.

Sorry it has take me so long to cycle back around to continue work on this one. I like to work on a few tickets so that I can post changes and wait for feedback, but I've allowed my working queue to grow too large recently! I appreciate you working on this one with me; I think there are some tough questions to figure out before we could push ahead with the changes.

First, a minor change,

• I've changed the name of the policy to ReadonlyWikiPolicy. When I first named it WikiPermissionPolicy, I was thinking that it could encapsulate potentially multiple wiki policies, but now I'm thinking that the policy should do one thing only, since it serves as a means to enable and disable the read-only wiki support. I foresee potential errors though when users mistakenly add ReadOnlyWikiPolicy (and for that matter, should it be ReadonlyWikiPolicy or ReadOnlyWikiPolicy?). The changes proposed in #10285 will help catch typos like this.

Replying to rjollos:

permission_policies = WikiPermissionPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy.

The ordering of permission_policies can be critical. Does it matter here, or should this work in any order?

ReadonlyWikiPolicy needs to come before DefaultPermissionPolicy because the latter will return True if the user has the permission (e.g. WIKI_MODIFY) and then ReadonlyWikiPolicy won't even be checked. If AuthzPolicy is enabled, ReadonlyWikiPolicy should come after AuthzPolicy, I think.

Maybe instead of return 'WIKI_ADMIN' in perm we should only if not 'WIKI_ADMIN' in perm: return False, so other policies can deny permissions for other reasons.

Yeah, that seems like a good idea. I've made that change in the latest, rjollos.git:t11244.2.

Another critical aspect is the perm check. Can there be any non-obvious behavior triggered by recursion?

I'm not seeing how this could come about. Do you have an example in mind?

It seems to me there should be no problems, at least with the default policies. But tests would for sure be appropriate, even if they can't prove the absence of problems with third-party policies.

You mention an environment upgrade step, I assume to automatically enable this policy. Is there a precedent for doing something like that? AuthzSourcePolicy was not added automatically. This case may be different of course. It seems saver to do so, but maybe there are scenarios with other policies where it's not?

That's a tough question. If we push this in 1.0.2 and a user upgrades to 1.0.2 without adding the ReadonlyWikiPolicy, all of their read-only wiki pages will suddenly be editable by users with WIKI_MODIFY. On the other hand, upgrading the environment is difficult for anything other than a default permission policy.

At this point, I'm thinking that the changes suggested here would be more appropriate for a development release such as milestone 1.1.2, and most users would pick up the change in Trac 1.2 when it would be more reasonable to expect the user to read the release notes and edit their permission_policies. The default for new installations could be ReadonlyWikiPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy.

Another idea would be to add some logging in trac.wiki.web_ui:process_request for the case that the wiki page has the read-only property but ReadonlyWikiPolicy isn't one of the permission_policies. I've implemented logging and a warning in the most recent changes.

What do you think?

I really like the changes. The templates don't have to take into account the readonly property when doing permissions checks. Everything is much simpler and I feel that these are the right change to make, we just need to take care for transitioning users with the change.

Once we have a clear path I'll add some functional tests.

[Peter Suter]

Replying to rjollos:

should it be ReadonlyWikiPolicy or ReadOnlyWikiPolicy?

Probably a matter of taste. (OT: ​.NET and ​Java seem to prefer ReadOnly. ​.NET conventions DO NOT capitalize each word in so-called closed-form compound words. But apparently their dictionary has the hyphenated form read-only, not the closed-form readonly.)

I'm not seeing how this could come about. Do you have an example in mind?

No. (This comment​ suggests we must be careful with testing 'WIKI_ADMIN' not in perm(resource), which can again call IPermissionPolicy.check_permission(). But we test action == 'WIKI_MODIFY' so there's no direct recursion. Theoretically if another policy does the same in reverse and checks for 'WIKI_MODIFY' in perm(resource) we could get infinite mutual recursion I think.)

At this point, I'm thinking that the changes suggested here would be more appropriate for a development release

Yes, this doesn't seem suitable for a stable branch.

I've implemented logging and a warning in the most recent changes.

The warning The readonly property is not be enforced... has a grammatical problem.

If leaking internal configuration is a concern, you could probably remove this part: because "ReadonlyWikiPolicy" is not in the list of active "permission_policies" as this is also mentioned in the log.

Maybe even the log should point to the documentation instead, which should explain important details like the required policy order.

[Ryan J Ollos]

Replying to psuter:

Maybe even the log should point to the documentation instead, which should explain important details like the required policy order.

Looking around the documentation, a new section on the TracPermissions page seems like the right place for a paragraph or two, as well as a brief mention on the TracWiki page, at least pointing to the relevant section on the TracPermissions page. Additionally it would probably be useful to add brief mentions of ReadonlyWikiPolicy on the TracFineGrainedPermissions and AuthzSourcePolicy pages.

Eventually I presume we'll have a section in TracUpgrade#a6.StepsspecifictoagivenTracversion on upgrading from 1.0 to 1.2. In the meantime, adding some documentation on TracDev/ReleaseNotes/1.1 might be most appropriate. Maybe a "caveats" section like TracDev/ReleaseNotes/1.0? How does that sound?

Thank you for the feedback. I'll incorporate your suggestions and post the revised changes.

[Peter Suter]

A new section in TracFineGrainedPermissions seems most fitting to me. Maybe also an additional new page ReadonlyWikiPolicy (but not part of the default pages) as a pointer there similar to AuthzSourcePolicy and AuthzPolicy.

TracPermissions so far does not go into much detail about policies. Maybe it's good to limit this page to the basics as an introduction. This could link to the new section in TracFineGrainedPermissions:

WIKI_ADMIN

All WIKI_* permissions, plus the management of readonly pages.

Or why would you prefer the new section here?

On the TracWiki page it could be linked in this part: Note that not all Trac wikis are editable by anyone, this depends on the local policy.

I'm not sure anything is needed on AuthzSourcePolicy, except updating the trac.ini snippet.

Perhaps you meant AuthzPolicy? (Since the policy order is important there.) But the documentation on that seems to be almost entirely in TracFineGrainedPermissions#AuthzPolicy instead.

Sections in TracUpgrade and TracDev/ReleaseNotes sound good.

[Ryan J Ollos]

Replying to psuter:

If leaking internal configuration is a concern, you could probably remove this part: because "ReadonlyWikiPolicy" is not in the list of active "permission_policies" as this is also mentioned in the log.

I'm in favor of removing that part, and I'm wondering if we should just remove the warning message entirely rjollos.git:f8291824. Initially I added it in there to give users an indication of why the read-only checkbox is not present, and so that the issue would be more obvious to both users and administrators. However, it is easy to argue that a warning message should not be shown for this same reason. It is effectively the same as telling a user that shouldn't have WIKI_MODIFY, "hey, you shouldn't be able to edit this page, but you can!". Which seems better to you?

[Peter Suter]

Replying to rjollos:

I'm wondering if we should just remove the warning message entirely

Indeed, removing it entirely seems most consistent with the reasoning. Again another possibility would be to show it only to WIKI_ADMIN, since only they would see the checkbox. I'm not even 100% convinced the log entry is worth keeping.

Theoretically, someone might want to write their own plugin that uses page.readonly and replaces ReadonlyWikiPolicy. Then the log message would get in the way more than it helps. (That might also be a minor argument against using page.readonly = None to request a hidden field instead of a separate variable show_readonly_checkbox.)

If the log message is kept, it might be helpful to add the name of the readonly page that triggered the warning, and (for people that really want to disable the feature) a hint that they can get rid of the warning by changing that page to "not readonly" somehow.

I could live with any choice. But when in doubt, I guess I would prefer keeping it simple.

[Ryan J Ollos]

Replying to psuter:

… Theoretically, someone might want to write their own plugin that uses page.readonly and replaces ReadonlyWikiPolicy. Then the log message would get in the way more than it helps. (That might also be a minor argument against using page.readonly = None to request a hidden field instead of a separate variable show_readonly_checkbox.)

Yeah, I'll change that - better to be explicit.

I could live with any choice. But when in doubt, I guess I would prefer keeping it simple.

I like the idea of keeping it simple, particularly since we are targeting 1.1.2. If early-adopters encounter trouble we can add the warning messages before Trac 1.2 is released.

[Ryan J Ollos]

Latest changes can be found in rjollos.git:t11244.4. Here is a summary of the changes:

• Removed [f8291824/rjollos.git].
• Added the explicit parameter show_readonly_checkbox.
• Added functional tests and unit tests. I've broken away from the pattern in the codebase of not having unit tests for the web_ui modules. Let me know if you see any problems with that.
• Changeset to address #11309.

[Ryan J Ollos]

Committed to trunk in [12099:12101].

[Ryan J Ollos]

I forgot to change the log messages when I rebased this work on the trunk, so the log messages for the commits that went directly to the trunk show 1.0.2dev: at the start of the string rather than 1.1.2dev:. Sorry about that!

[Ryan J Ollos]

Although, now that I think about it, I think I meant to commit [12099:12100] to 1.0-stable, so maybe I should just merge those over.

[Thijs Triemstra]

r12101 introduced a bad import:

2013-09-25 19:43:46,216 Trac[loader] ERROR: Skipping "trac.mimeview.pygments = trac.mimeview.pygments [pygments]": 
Traceback (most recent call last):
  File "c:\users\thijs\workspaces\trac\trac\loader.py", line 68, in _load_eggs
    entry.load(require=True)
  File "C:\Python27\lib\site-packages\distribute-0.6.35-py2.7.egg\pkg_resources.py", line 2015, in load
    entry = __import__(self.module_name, globals(),globals(), ['__name__'])
  File "c:\users\thijs\workspaces\trac\trac\mimeview\pygments.py", line 23, in <module>
    from trac.tests import compat
  File "c:\users\thijs\workspaces\trac\trac\tests\__init__.py", line 16, in <module>
    from trac.tests import attachment, config, core, env, perm, resource, \
  File "c:\users\thijs\workspaces\trac\trac\tests\wikisyntax.py", line 24, in <module>
    from trac.wiki.tests import formatter
  File "c:\users\thijs\workspaces\trac\trac\wiki\tests\__init__.py", line 20, in <module>
    from trac.wiki.tests import formatter, macros, model, web_ui, wikisyntax
ImportError: cannot import name web_ui

[Ryan J Ollos]

Oh, I missed the add of that file when pushing the changes from Git to SVN. Thanks for catching that. Committed in [12102].

It seems last night was not a good night for checking in code!

[Ryan J Ollos]

Merged [12099:12100] to 1.0-stable in [12106], and record-only merge of [12106] on the trunk in [12107].

[Ryan J Ollos]

Added remove_config to FunctionalTestEnvironment in [12108] and merged to trunk in [12109]. This will be utilized in #10010.

[Ryan J Ollos]

Replying to rjollos:

Merged [12099:12100] to 1.0-stable in [12106], and record-only merge of [12106] on the trunk in [12107].

[12106] resulted in svn:mergeinfo for /trunk being added to branches/1.0-stable​: Property svn:mergeinfo changed /trunk (added). Since the normal pattern is to merge from 1.0-stable → trunk, should I just go ahead and delete this property?

[Christian Boos]

Replying to rjollos:

… Property svn:mergeinfo changed /trunk (added). Since the normal pattern is to merge from 1.0-stable → trunk, should I just go ahead and delete this property?

Well, not the whole property of course, just that added /trunk line (svn pedit helps for doing mergeinfo surgery ;-) ).

[Ryan J Ollos]

Replying to cboos:

Well, not the whole property of course, just that added /trunk line (svn pedit helps for doing mergeinfo surgery ;-) ).

Thanks, fixed in [12110].

[Ryan J Ollos]

Wiki documentation has been updated:

• AuthzSourcePolicy changes
• TracFineGrainedPermissions changes
• TracUpgrade changes
• ReadonlyWikiPolicy added

Please edit or let me know if you spot areas for improvement. I'm rather bad at spotting my own grammar errors.

As I've thought about this some more, it might be useful to have an upgrade step that checks for the default permission_policy (DefaultPermissionPolicy, LegacyAttachmentPolicy) and adds ReadonlyWikiPolicy for that case. For other cases, it could echo a warning to the user. At this point though, I'm inclined to leave the changes as they are and see if we get many reports of users experiencing trouble.

[Peter Suter]

The documentation updates look good!

An automatic upgrade would be nice. But the user should still be allowed to remove the policy. How would the upgrade step know it shouldn't remove it again?

[Ryan J Ollos]

Replying to psuter:

An automatic upgrade would be nice. But the user should still be allowed to remove the policy. How would the upgrade step know it shouldn't remove it again?

I was thinking that we could utilize an upgrade script, which associates the upgrade step with a system.database_version number so that the upgrade only happens once. tags/trac-1.0.1/trac/upgrades/db29.py​ appears to do a similar operation.

[Peter Suter]

Replying to rjollos:

I was thinking that we could utilize an upgrade script

Right. (I assumed this reasoning still applied, so db upgrade scripts should only be used for db schema changes. But it looks like that's outdated. The discussion of upgrades db28 and db29 not only suggests reusing the upgrade scripts for other changes is acceptable, but maybe even generally preferable to separate upgrade components.)

[Ryan J Ollos]

I've added an upgrade step in the proposed changes: log:rjollos.git:t11245.5.

[Jun Omae]

Replying to rjollos:

I've added an upgrade step in the proposed changes: log:rjollos.git:t11245.5.

I've tried it out. LGTM. But I have two suggestions.

1. We could use env.config.getlist() instead of env.config.get() and .split()
2. Extracted message has a leading newline

trac/upgrades/db30.py

@@ -27 +27 @@
     the default value. Otherwise, echo a message about the need to manually
     add ReadonlyWikiPolicy to the list of permission_policies."""
 
-    policies = [p.strip() for p in
-                env.config.get('trac', 'permission_policies').split(',')]
+    policies = env.config.getlist('trac', 'permission_policies')
     if policies == _old_default:
         backup_config_file(env, '.db30.bak')
         env.config.set('trac', 'permission_policies', ', '.join(_new_default))
@@ -37 +36 @@
     elif policies != _new_default:
         # TRANSLATOR: Wrap message to 80 columns
         env.log.info("ReadonlyWikiPolicy must be manually enabled.")
-        printout(_("""
+        printout(_("""\
 To enable the readonly wiki attribute, trac.ini must be manually edited to
 add ReadonlyWikiPolicy to the list of permission_policies in the [trac]
 section.

[Ryan J Ollos]

Thanks for the testing and feedback. The newline was actually intentionally. I was trying to make the text stand-out so it has a greater chance of being noticed. There is also a newline at the end of the text block. Maybe there is a way to add the newline without interfering with the translation?

[Jun Omae]

Replying to rjollos:

The newline was actually intentionally. …

Ah, sorry, no matter if that's intent. I have not thought that's intent because a message doesn't a leading newline in similar db28.py.

[Ryan J Ollos]

It could be better to prefix the line with something like INFO:. I worry that some users won't notice the output. What do you think?

[Jun Omae]

Fine by me, but I like Information: prefix rather than it. Warning: prefix already is used at trac/env.py:1000​ and trac/admin/console.py:476​.

[Jun Omae]

I found a minor issue in db30.py. If run upgrade with the following

permission_policies = ReadonlyWikiPolicy, AuthzPolicy, DefaultPermissionPolicy, LegacyAttachmentPolicy

trac-admin says the following.

Trac [/var/trac/0.13-t11244]> upgrade
To enable the readonly wiki attribute, trac.ini must be manually edited to
add ReadonlyWikiPolicy to the list of permission_policies in the [trac]
section.

For more details see: http://trac.edgewall.org/wiki/ReadonlyWikiPolicy

Upgrade done.

trac/upgrades/db30.py

@@ -34 +34 @@
         env.config.set('trac', 'permission_policies', ', '.join(_new_default))
         env.config.save()
         env.log.info("Enabled ReadonlyWikiPolicy.")
-    elif policies != _new_default:
+    elif 'ReadonlyWikiPolicy' not in policies:
         # TRANSLATOR: Wrap message to 80 columns
         env.log.info("ReadonlyWikiPolicy must be manually enabled.")
         printout(_("""

[Ryan J Ollos]

Thanks for catching the issue in comment:36. I've added the prefix Notice: to the string and included all the suggested changes in log:rjollos.git:t11245.5.

[Ryan J Ollos]

Upgrade step committed to trunk in [12253:12254].

[Ryan J Ollos]

Replying to rjollos:

Wiki documentation has been updated:

• AuthzSourcePolicy changes
• TracFineGrainedPermissions changes
• TracUpgrade changes
• ReadonlyWikiPolicy added

I've edited the documentation to mention the environment upgrade:

• TracFineGrainedPermissions
• TracUpgrade
• ReadonlyWikiPolicy

[Ryan J Ollos]

Replying to rjollos:

Upgrade step committed to trunk in [12253:12254].

In [13386], fixed hint to translators not being included in the pot file.

[Ryan J Ollos]

#12719 is a continuation of the work in this ticket, and replaces ReadonlyWikiPolicy with DefaultWikiPolicy.

[Ryan J Ollos]

From r12108, remove_config should not return a value.

trac/tests/functional/testenv.py

@@ -225 +225 @@
     def remove_config(self, *args):
         """Calls trac-admin to remove the value for the given option
         in `trac.ini`."""
-        return self._tracadmin('config', 'remove', *args)
+        self._tracadmin('config', 'remove', *args)
 
     def _tracadmin(self, *args):
         """Internal utility method for calling trac-admin"""

[Ryan J Ollos]

comment:2 change applied in r17398 from 1.0-stable, merged in [17399:17401].