search

mikitex70 / redmine_drawio

Macro plugin to embed draw.io diagrams into Redmine wiki pages
MIT License
123 stars 51 forks source link

Fixed usage of drawio_svg_enabled setting #106

Closed liaham closed 2 years ago

liaham commented 2 years ago

If svg is disabled in plugin settings the corresponding checkbox in _marcro_dialog.html.erb won't be displayed anymore. Before that fix it was always displayed.

Adds also tests for this fix! Furthermore, a check for the settings table is added in DrawioSettingsHelper.svg_enabled? since the table is not immediately available when loading the code for tests.

mikitex70 commented 2 years ago

Hi @liaham, thanks for your PR, especially for the tests: I'm not a Ruby developer so I need to study how to implement them, and your code is a good start point. The CHANGELOG.md is generated from commit messages by a Python utility (gitchangelog) so there is no need to update it manually.

I've only a doubt about your work: when disabling the SVG format for diagrams, only the rendering is changed, the svg images are inserted in the HTML as data: images so they have the same quality of the original SVG, only hyperlinks in it are not working. So the svg format in the dialog is still useful, if someone wants the SVG image quality, especially when images are scaled. Now the question: do you think is better to not allow at all the SVGs if they are disabled by the administrator? Surely an SVG attachment can be potentially dangerous, but with the editor is not possible to save malicious diagrams (at least as far as I know). The only way is to manually upload it as an attachment or a document, and this is independent from the plugin. This is only to know your (and others) opinion about this use case, and to decide if hiding the radio button is the right choice.

liaham commented 2 years ago

Hi @mikitex70,

I am happy that you appreciate the tests. I 've hopped that they would encourage to add some more. :)

This is this my opinion concerning the SVG format:

Both, a checkbox named Enable SVG diagramms and the condition concerning svg_enabled in the macro_dialog view, implies that SVG is available if checked or is not available if not checked. Together with the warning below the checkbox, nobody would expect that one could nevertheless include SVGs.

If an administrator decides to use SVG beeing aware of their inherent risk than she should be able to do so. But when an administrator decides for the most secure option and disables SVG then they should be fully disabled. This is more consequent than disabling SVG only 'a little bit'.

If you prefer to provide always SVG then the checkbox should be removed or renamed to something more meaningful as long as the setting will be used anywhere. But as far as I could see, does it not control directly any Javascript. Even in the current code does it not control anything since you set svg_enabled to true when its nil.

Please let me know if I am wrong in any point.

mikitex70 commented 2 years ago

You convinced me, your point of view seems correct to me. I would merge your PR as soon as possible, maybe tomorrow.

liaham commented 2 years ago

I am very pleased to hear that. :) Thank you!