dokuwiki-plugin-struct icon indicating copy to clipboard operation
dokuwiki-plugin-struct copied to clipboard

Add option to restrict users/groups that can autocomplete usernames

Open cmacmackin opened this issue 1 year ago • 7 comments

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

cmacmackin avatar Dec 02 '23 10:12 cmacmackin

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.

annda avatar Dec 11 '23 14:12 annda

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 avatar Dec 11 '23 23:12 cmacmackin

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

cmacmackin avatar Dec 11 '23 23:12 cmacmackin

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.

annda avatar Dec 12 '23 10:12 annda

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.

cmacmackin avatar Dec 12 '23 11:12 cmacmackin

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

splitbrain avatar Dec 12 '23 11:12 splitbrain

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?

cmacmackin avatar Dec 12 '23 11:12 cmacmackin