Opened 8 years ago

Closed 8 years ago

menu item displayed as strange artifact when having no permission for it

Reported by: Owned by: falkb Ryan J Ollos normal MenusPlugin normal Ryan J Ollos 0.12

Description

Users without permission BROWSER_VIEW (and TIME_VIEW of TimingAndEstimationPlugin) actually should not see the appropriate menu items.

But they see this here:

I added rjollos because it seems he's the current maintainer.

comment:1 follow-up:  4 Changed 8 years ago by falkb

We found out the reason seems to be this setup in trac.ini:

[mainnav]
billing = enabled
browser = enabled


Now as I removed both entries for billing and browser, the problem disappeared.

It seems, the permissions are not checked correctly at rendering menu items...

comment:2 Changed 8 years ago by Ryan J Ollos

Owner: changed from Catalin BALAN to Ryan J Ollos new → assigned

comment:3 Changed 8 years ago by Ryan J Ollos

(In [12144]) Refs #10133: Renamed 0.11 directory to trunk.

comment:4 in reply to:  1 ; follow-up:  8 Changed 8 years ago by Ryan J Ollos

It seems, the permissions are not checked correctly at rendering menu items...

I can't think of how the permission could be checked in a straightforward way, since all you've done is defined an entry, but no path_info or other data to match it to a module. How does the MenusPlugin know, in a general way, that the browser entry in trac.ini is associated with the BrowserModule and the path /browser, and therefore should only be shown when the BROWSER_VIEW permission is present? Do you see a way?

However, I will commit a fix that prevents the display of entries that don't result in a valid link element.

comment:5 Changed 8 years ago by Ryan J Ollos

Resolution: → fixed assigned → closed

(In [12145]) Fixes #10133: Don't show entries in the navigation bar if a label with a link element is not defined.

comment:6 Changed 8 years ago by Ryan J Ollos

(In [12146]) Refs #10133: Removed unnecessary continue statements.

comment:7 Changed 8 years ago by Ryan J Ollos

(In [12147]) Refs #10133: Added else case so that logic is the same as before [12145].

comment:8 in reply to:  4 Changed 8 years ago by falkb

Do you see a way?

No, I don't

However, I will commit a fix

Thanks!

comment:9 Changed 8 years ago by Ryan J Ollos

Let me know if you've had a chance to test it out. I'm hesitant to touch the plugin again without setting up unit tests to make sure I'm not introducing regressions.

comment:10 Changed 8 years ago by falkb

Tested with 0.12 and 1.0, it works well.

Modify Ticket

Change Properties