Disabling SearchModule does not remove search box

[imagotrigger@…]

naturally you get a "No handler matched request to /search"

[Jun Omae]

Reproduced. Workaround is to revoke SEARCH_VIEW from all users before disabling SearchModule.

We should check whether SearchModule is enabled before 'SEARCH_VIEW' in perm like tags/trac-1.0.1/trac/ticket/query.py@:1094#L1088​, or should fix PermissionSystem to ignore actions which are no longer defined via IPermissionRequestor.

[anonymous]

Thanks for the quick reply!

[Ryan J Ollos]

Replying to jomae:

Reproduced. Workaround is to revoke SEARCH_VIEW from all users before disabling SearchModule.

We should check whether SearchModule is enabled before 'SEARCH_VIEW' in perm like tags/trac-1.0.1/trac/ticket/query.py@:1094#L1088​,

I was considering that, but would it be reasonable for a plugin to replace SearchModule and utilize the search box?

or should fix PermissionSystem to ignore actions which are no longer defined via IPermissionRequestor.

So 'SEARCH_VIEW' in perm would return False? That sounds like a very interesting idea which might elegantly solve the problem. It would be nice to avoid those Environment.is_component_enabled tests in the conditional.

[Ryan J Ollos]

With the changes in log:rjollos.git:t11748, DefaultPermissionPolicy will only return True if the action is defined. For the cases in which an action is defined on the "Module" class (the class implementing IRequestHandler), we can remove several checks for Environment.is_component_enabled and retain the same behavior (e.g. ReportModule and BatchModifyModule).

As with the change in #10285 for components listed in permission_policies, we are now "failing closed" for the case that an action is no longer defined.

This also fixes the problem with the search box being present when SearchModule is disabled. The search box should still be present when a plugin implements a handler for the /search path and defines the SEARCH_VIEW permission (untested).

The changes are implemented on the trunk and all tests are currently passing. We could implement on 1.0-stable, but we should consider that carefully. More testing is needed, but I'm feeling positive about this change.

[Ryan J Ollos]

Replying to rjollos:

As with the change in #10285 for components listed in permission_policies, we are now "failing closed" for the case that an action is no longer defined.

The situation may be a bit different from #10285 though since in that case there was a security concern. I haven't yet thought of a way in which the current behavior of granting permission for an action that isn't valid could be a security risk.

[Ryan J Ollos]

Replying to rjollos:

With the changes in log:rjollos.git:t11748, DefaultPermissionPolicy will only return True if the action is defined. For the cases in which an action is defined on the "Module" class (the class implementing IRequestHandler), we can remove several checks for Environment.is_component_enabled and retain the same behavior (e.g. ReportModule and BatchModifyModule).

Modifying IPermissionRequestor is not a good solution since other permission_policies could be used. I think [825b229f/rjollos.git] is a better solution, however it requires that all permissions be defined in IPermissionRequestor implementations. That's not the case for ATTACHMENT_VIEW, ATTACHMENT_CREATE and ATTACHMENT_DELETE. If we register those in an IPermissionRequestor implementation we need a way to keep the permissions from being visible on the Permission admin page, in TracAdmin permission list, etc…

There needs to be a way to distinguish regular permissions from these special legacy permissions, such as prefixing legacy permissions with a leading underscore. Or we just get rid of the LegacyAttachmentPolicy and require that all permissions be defined in an IPermissionRequestor.

WiP in log:rjollos.git:t11748.2 (at least one unit test fails due to noted issues).

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Narrowing focus for milestone:1.2. Please move ticket to milestone:1.2 if you intend to fix it.

[Ryan J Ollos]

Possible solution is to have PermissionSystem.get_user_permissions only return permissions defined in IPermissionRequestor implementations: log:rjollos.git:t11748.3

This is a minor API change which could be avoided by having undefined default to True. However, I'm trying to keep the permission checking logic in PermissionSystem so that the behavior doesn't depend on the IPermissionStore being used.

[Ryan J Ollos]

Committed to trunk in r15090.

[Ryan J Ollos]

After r15090 the warning in web_ui​ will never be logged because undefined permissions are not returned from the call to ps.get_user_permissions(group). We could use the undefined=True parameter, but the warning doesn't appear to be very useful so I removed it in r16332.

When checking whether the user has permission to grant a permission, meta actions should not be expanded because it results in a confusing error message as shown in the r16332 test case (the message references WIKI_DELETE rather than WIKI_ADMIN). I'll fix that in #12915.