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

Add option to restrict users/groups that can autocomplete usernames #695

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cmacmackin
Copy link
Contributor

A configuration "allow_username_autocomplete" has been added. This is a list of usernames and/or groups to which to limit the availability of autocomplete for username type data. By default this is set to @ ALL (space included in this message so I don't send notifications to every repository contributor), so the behaviour is backwards-compatible. Tests of this new feature have been added. I've only added language data for English, as that is the only language I speak. Contributions for other languages would be more than welcome.

Fixes #690

@annda
Copy link
Contributor

annda commented Dec 11, 2023

Thanks for the PR.

However, this implementation may not work if the plugin config is not loaded at the time the check takes place. You cannot detect it in your tests, because you manually set the value in the config array. So in a way you simulate a partial config load, which may not yet have happened in a normal request.

I think you can avoid this problem by introducing a wrapper getConf() method in the AbstractBaseType, similar to the existing convenience method getLang(). This would internally load the config if needed.

This can cause errors if the configurations haven't been initialised
yet. On the suggestion of annda, I've added a getConf() method which
will load the configurations if they haven't been already.
@cmacmackin
Copy link
Contributor Author

Thanks for the feedback. My ignorance of the internal workings of DokuWiki is showing. I've added a getConf() method as you suggest. Is there a better way to write my tests so that they'd be able to catch problems such as these?

@cmacmackin
Copy link
Contributor Author

The test failures here all seem to have arisen due to recent changes to master.

@annda
Copy link
Contributor

annda commented Dec 12, 2023

Thanks for the changes, the code is more robust now.

On second thought, and seeing how cumbersome it is to handle the config in tests, is there a reason why you want to introduce a plugin setting? It would certainly be much easier and also more flexible to handle this in autocomplete configuration.

@cmacmackin
Copy link
Contributor Author

On second thought, and seeing how cumbersome it is to handle the config in tests, is there a reason why you want to introduce a plugin setting? It would certainly be much easier and also more flexible to handle this in autocomplete configuration.

I'm not sure that really makes sense from a security perspective. If an admin doesn't want some users being able to see a list of usernames in one schema, surely that would apply to all schemas. Requiring them to configure that for every User column in every schema creates the opportunity for them to forget one.

@splitbrain
Copy link
Member

I think I would want to set this based on context. A schema used by the marketing department would probably only want to have marketing people in the autocomplete. A schema for bug reports might want to use all the developers in one field (assignee) but all users in another (reporter)...

@cmacmackin
Copy link
Contributor Author

cmacmackin commented Dec 12, 2023

I think I would want to set this based on context. A schema used by the marketing department would probably only want to have marketing people in the autocomplete. A schema for bug reports might want to use all the developers in one field (assignee) but all users in another (reporter)...

That's a different thing (though obviously useful). That is about who shows up in the autocomplete list. This PR is about who is allowed to see the autocomplete list. I suppose if the former were available then it might make sense to also have the latter be locally-configurable. Would you this PR be changed to leave that possibility open?

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 this pull request may close these issues.

Autocomplete security for User datatype
3 participants