Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACL isn't being respected on IndexMenu2 (TreeNew) #309

Open
eduardomozart opened this issue Mar 28, 2024 · 7 comments · May be fixed by #310
Open

ACL isn't being respected on IndexMenu2 (TreeNew) #309

eduardomozart opened this issue Mar 28, 2024 · 7 comments · May be fixed by #310

Comments

@eduardomozart
Copy link
Contributor

eduardomozart commented Mar 28, 2024

Hello,
When using the IndexMenu2 (TreeNew), it isn't respecting the ACL restrictions and still shows unaccessible items on sidebar, as can be seen below.

GravacaodeTela2024-03-28as10 49 15-ezgif com-video-to-gif-converter

The ACL doesn't allows the user to access the "3cx", "ad", "airwave", "apache", "cacti", "central", "conceitos" and "db" ns's are being shown. When expanding them, they shows the subdirectory structure only (it's not possible to click or enter on them) - the pages itself respect the ACL and aren't being shown.

The "Aruba ClearPass", "Aruba Instant AP" and "Aruba Mobility" namespaces, the user has access to and they show the namespaces and page files as expected, but inside "Aruba ClearPass", the user has no access to "labguide" subns but it's directory structure are still shown similar to the ns's cited above.

Here's how the ACL is set to "Aruba ClearPass" ns and the other ones (set to the group that this user belongs - the user belongs to "@Lettel" and "@user" groups):

*	@user	0
sidebar	@ALL	0
start	@user	1
arubavmc	@lettel	1
arubavmc:*	@lettel	1
zabbix	@lettel	1
zabbix:*	@lettel	1
arubavmc:labguide	@lettel	0
arubavmc:labguide:*	@lettel	0
iap:*	@lettel	1
iap	@lettel	1
clearpass:*	@lettel	1
clearpass:labguide	@lettel	0
clearpass:labguide:*	@lettel	0
formalms:*	@lettel	1
formalms	@lettel	1
pfsense:*	@lettel	1
pfsense	@lettel	1
sidebar	@lettel	1
clearpass	@lettel	1
@eduardomozart eduardomozart changed the title ACL doesn't being respected on IndexMenu2 (TreeNew) ACL isn't being respected on IndexMenu2 (TreeNew) Mar 28, 2024
eduardomozart added a commit to eduardomozart/indexmenu that referenced this issue Mar 28, 2024
@eduardomozart eduardomozart linked a pull request Mar 28, 2024 that will close this issue
@eduardomozart
Copy link
Contributor Author

After applying the PR #310 the IndexMenu TreeNew is shown as expected:
image

@Klap-in
Copy link
Collaborator

Klap-in commented Mar 28, 2024

The fix is now in the latest step that prepares the data that must be outputted for the treenew. I'm surprised that $data array in makeNodes contains entries that doesn't respect ACL. The step before that actually generates that $data array should already check this...
Unfortunately, I have now no time to check in detail.

@Klap-in
Copy link
Collaborator

Klap-in commented Aug 4, 2024

Do I understand it correctly that pages which are forbidden by ACL are hidden, but the namespaces containing them are not?
I assume you use https://www.dokuwiki.org/config:sneaky_index is off.
Default DokuWiki shows namespaces while pages are forbidden for acl. If a folder has subfolders, it is more difficult to determine if it has any subnamespace or child pages that might be permitted. Therefore, DokuWiki default shows the namespaces. If it is easy to know that no child nodes are visible, current logic of Indexmenu hides them already (e.g. node with only pages).

I rewrote the code here a bit, so the behaviour might be changed, but my expectation was that the behaviour is still the same as before. If you use the old tree, then these namespaces were also shown?

@Klap-in
Copy link
Collaborator

Klap-in commented Aug 4, 2024

As one of the reasons why indexmenu cannot see if lower child nodes are allowed or not is that it does not load lower nodes, this is to make the lazy loading more efficient.
Eventually, you can use max#n to retrieve with AJAX more levels at once. Then it will see for more levels if these are empty, however, if you have very deep namespace levels, you might need a high number or accept sometimes empty nodes are still shown.

@eduardomozart
Copy link
Contributor Author

I believe the ACL check wouldn't take so much time to be made, it loads in chuncks and only when user expands a node by default, so it shouldn't be an issue so my fix seems to be reasonable to do into the end instead of wherelse it's being doing right now. I don't want to use max#n because I don't know how many subnamespaces a Wiki can have, and it's better to compromise performance then deal with security concerns (user being able to see namespaces they shouldn't).

@Klap-in
Copy link
Collaborator

Klap-in commented Aug 15, 2024

@eduardomozart
Copy link
Contributor Author

Hello @Klap-in,
Sorry for the late reply. Yes, when using the treeold the IndexMenu handles the ACL correctly and namespace not available for the user isn't being shown.
I'm not using sneaky_index, but enabling it seems to fix the issue. Not sure if it's intended and you'll close this ticket or if you'll explore it further, but I still believe that ACL check should be made after or the code reviewed to make sure that ACL checking will be made as expected to avoid this issue for another users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants