GNOME Bugzilla – Bug 735675
Lockdown settings enhancement: disks-no-change.page created
Last modified: 2014-11-25 18:40:51 UTC
Hello, I've created a page for system-admin-guide, user settings. This page explains how to prevent users from changing disks partition/format. Any reviews and comments are very welcome. Thanks, jana
Created attachment 284822 [details] prevent users to change disks settings
Created attachment 285291 [details] [review] patch to add page Patches are easier to review, so I created a patch from your page.
Review of attachment 285291 [details] [review]: Page should be called something like lockdown-repartion.page. A license is missing from the page. ::: system-admin-guide/C/no-change-disks.page @@ +2,3 @@ + xmlns:its="http://www.w3.org/2005/11/its" + type="topic" style="task" + id="no-change-disks"> Odd indentation here. @@ +7,3 @@ + <link type="guide" xref="user-settings#lockdown"/> + <revision pkgversion="3.11" date="2014-08-21" status="draft"/> + <link type="seealso" xref="dconf-lockdown" /> As you link to dconf, it would be good to stub out a page about polkit. @@ +20,3 @@ + <title>Lock down changing disks settings</title> + + <p><sys>PolicyKit</sys> enables you to set permissions for individual operations. You probably mean polkit, not PolicyKit, as PolicyKit is not the correct term any longer (I think that the name change was made at the same time as an API change, a few years ago). @@ +30,3 @@ + <title>To prevent users from changing disks settings:</title> + <item><p>Create a file with the same content as in <file>/usr/share/polkit-1/actions/org.freedesktop.udisks2.policy</file>.</p> + <code>cp /usr/share/polkit-1/actions/org.freedesktop.udisks2.policy /etc/share/polkit-1/actions/org.freedesktop.udisks2.policy</code> If not here, it would be useful to mention that polkit configuration in /etc overrides that shipped by packages in /usr/share. Sysadmins should know this, but it would be worth mentioning it in a bakground page. @@ +40,3 @@ +<screen><![CDATA[ + <action id="org.freedesktop.udisks2.modify-device"> + <description xml:lang="en_GB">Modify the disks settings</description> The xml:lang attribute seems unnecessary. @@ +49,3 @@ + </action> +]]></screen> + <p>Replace <code>no</code> vy <code>auth_admin</code> if you want to ensure "by"? @@ +57,3 @@ + <code>Authentication is required to modify the disks settings</code> + + </page> Indentation on the page element.
Created attachment 288534 [details] [review] lockdown-repartitioning.page created Dear David, Thank you for the comments, I've implemented them all. What do you think about the status of the page now? Am I allowed to push? Jana
Review of attachment 288534 [details] [review]: Just minor fixes needed, and a review from someone more familiar with the sysadmin guide, but you have made some good improvements. ::: system-admin-guide/C/lockdown-repartitioning.page @@ +17,3 @@ + <include href="legal.xml" xmlns="http://www.w3.org/2001/XInclude"/> + + <desc>This page explains how to prevent users from changing disks partition.</desc> "disks partition" would be better as "disk partitions". @@ +46,3 @@ + <screen><![CDATA[ + <action id="org.freedesktop.udisks2.modify-device"> + <description="en_GB">Modify the disks settings</description> I think that the '="en_GB"' is buogus, so probably best to just remove it. If you want to discuss localized descriptions and messages, that would be better off in a separate page about polkit actions.
Created attachment 291408 [details] [review] David's comments implemented into lockdown-partitioning.page Hello David, Once again thank you for your time. I've implemented your valuable comments; see the patch. Shall I close this bug? jana
Review of attachment 291408 [details] [review]: It seems that this is a patch of your previous patch. It would be better if you could squash the two patches together (with "git rebase -i HEAD~2" for the top 2 commits on a branch, then use "fixup" on the second patch to meld it into the first one. Feel free to ask me for guiadance if you are unsure about this). ::: system-admin-guide/C/lockdown-repartitioning.page @@ +17,3 @@ <include href="legal.xml" xmlns="http://www.w3.org/2001/XInclude"/> + <desc>This page explains how to prevent users from changing disk partitions.</desc> Much better, thanks! @@ +46,2 @@ <screen><![CDATA[ <action id="org.freedesktop.udisks2.modify-device"> It looks like you removed the description entirely. I think it is fine to have the description (and should be encouraged) but the '="en_GB"' looked bogus to me, so it is just that bit that should be removed.
Created attachment 291488 [details] [review] this patch contains the final version lockdown-repartitioning.page Here's the final version of lockdown-repartitioning.page in a patch containing all the fixes - see above. If you agree the page is perfect, I'll push to master. Thank you all, jana
Review of attachment 291488 [details] [review]: Looks good, please push!
Review of attachment 291488 [details] [review]: Tidied up and pushed to master as 14264e58c0fd64226a02a79d18c6c66ee1c52d36.