-
Notifications
You must be signed in to change notification settings - Fork 39
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
base: master
Are you sure you want to change the base?
Add option to restrict users/groups that can autocomplete usernames #695
Conversation
By default this is set to @ALL, so the behaviour is backwards-compatible. Tests of this new feature have been added.
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 |
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.
Thanks for the feedback. My ignorance of the internal workings of DokuWiki is showing. I've added a |
The test failures here all seem to have arisen due to recent changes to master. |
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. |
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. |
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? |
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