Edgewall Software
Modify

Opened 5 years ago

Closed 3 years ago

Last modified 2 years ago

#11748 closed defect (fixed)

Disabling SearchModule does not remove search box

Reported by: imagotrigger@… Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.1
Component: search system Version: 1.0-stable
Severity: minor Keywords:
Cc: Branch:
Release Notes:

The search box is only visible when the SEARCH_VIEW permission is defined and granted to the user.

API Changes:

PermissionSystem.get_user_permissions returns only defined permissions, unless the undefined parameter is True. The default value of undefined is False.

Description

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

Attachments (0)

Change History (14)

comment:1 by Jun Omae, 5 years ago

Keywords: Search removed
Milestone: unschedulednext-stable-1.0.x

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.

comment:2 by anonymous, 5 years ago

Thanks for the quick reply!

in reply to:  1 comment:3 by Ryan J Ollos, 5 years ago

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.

comment:4 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos added

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.

in reply to:  4 comment:5 by Ryan J Ollos, 5 years ago

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.

comment:6 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos removed
Milestone: next-stable-1.0.x1.1.5
Owner: set to Ryan J Ollos
Status: newassigned

in reply to:  4 comment:7 by Ryan J Ollos, 5 years ago

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

comment:8 by Ryan J Ollos, 5 years ago

Milestone: 1.1.51.2

comment:9 by Ryan J Ollos, 4 years ago

Milestone: 1.21.1.6

Milestone renamed

comment:10 by Ryan J Ollos, 4 years ago

Milestone: 1.1.6next-dev-1.1.x
Owner: Ryan J Ollos removed
Status: assignednew

comment:11 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.xnext-dev-1.3.x

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

comment:12 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Milestone: next-dev-1.3.x1.3.1
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: newassigned

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.

comment:13 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in r15090.

comment:14 by Ryan J Ollos, 2 years ago

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.

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

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