Project

General

Profile

Feature #26964

"Apply default permissions" in SMB share config is confusing

Added by Andrew Walker almost 3 years ago. Updated over 2 years ago.

Status:
Done
Priority:
Important
Assignee:
Erin Clark
Category:
GUI (new)
Estimated time:
Severity:
New
Reason for Closing:
Reason for Blocked:
Needs QA:
Yes
Needs Doc:
No
Needs Merging:
No
Needs Automation:
No
Support Suite Ticket:
n/a
Hardware Configuration:

Description

The current behavior of the checkbox is a problem because when a user is presented with a window with the "apply default permissions" box checked, he cannot know whether a destructive operation will occur.

Current behavior
Permissions get reset if the value of cifs_default_permissions in the freenas-v1.db file changes to 1. This triggers a winacl reset. The value of cifs_default_permissions stays 1 after the permissions are reset.

Better behavior
I think that (1) box should default to being unchecked (2) UI box being checked should always result in permissions reset (3) warning popup about the permissions being reset (4) after permissions are reset, the db value should also be reset to 0 (ie uncheck the box).


Related issues

Related to FreeNAS - Bug #27881: Set the default setting for the SMB -> Sharing -> SMB -> Apply Default Permissions checkbox to checkedDone
Related to FreeNAS - Bug #33084: The "Default Permissions" box is not checked when a new SMB share is createdDone
Related to FreeNAS - Bug #41784: Default permissions button gets untickedClosed

Associated revisions

Revision 84e4d8ec (diff)
Added by Erin Clark over 2 years ago

Have default permissions checked when creating an smb share as per discussion Ticket: #33084 #26964

History

#1 Updated by Andrew Walker almost 3 years ago

I believe that Caleb had a ticket in which UI confusion about this led to the customer accidentally resetting permissions on his samba share.

#2 Updated by Timur Bakeyev almost 3 years ago

It's a bit even more confusing:

        if self.instance._original_cifs_default_permissions != \
            self.instance.cifs_default_permissions and \
            self.instance.cifs_default_permissions is True:

It has to change from the previous state and it should be On. So not every time when it is checked the permissions are reset.

Basically, you need to uncheck it, save, then open dialog again and check the knob.

#3 Updated by Timur Bakeyev almost 3 years ago

I think, there is no need to keep this value in the DB at all. It should be 0 all the time, checking the box should trig reset on the dialog "Ok" and immediately set in DB the value to 0.

#4 Updated by Dru Lavigne almost 3 years ago

  • Status changed from Untriaged to 50

#5 Updated by Ash Gokhale almost 3 years ago

  • Status changed from 50 to 51
  • Priority changed from No priority to Important
  • Target version set to 11.1-U1

Current behavior is destructive, I support 1,2 and 4 unconditionally, dropping the model persistence ( migration dangers aside) and propose moderating 3 from a popup into tooltip with a frank warning about paving the acls'.

#6 Updated by Dru Lavigne almost 3 years ago

  • Status changed from 51 to Unscreened
  • Assignee changed from Ash Gokhale to Timur Bakeyev

#7 Updated by Timur Bakeyev almost 3 years ago

  • Status changed from Unscreened to Screened

#8 Updated by Timur Bakeyev almost 3 years ago

Ash, with the average user I think tooltip isn't intrusive enough to stop them from accidentally destroying the share permissions.

Also, not sure, how to implement that in the new UI...

#9 Updated by Dru Lavigne almost 3 years ago

  • Category changed from OS to GUI
  • Status changed from Screened to Unscreened
  • Assignee changed from Timur Bakeyev to Erin Clark
  • Target version changed from 11.1-U1 to TrueNAS 11.1-U2

Passing to Erin for the new UI. Erin, discuss with Timur any info you need for the implementation.

#10 Updated by Erin Clark almost 3 years ago

  • Status changed from Unscreened to Screened

#11 Updated by Erin Clark almost 3 years ago

  • Target version changed from TrueNAS 11.1-U2 to 11.2-BETA1

Should it be always not checked even for new shares? Also, what specifically should the warning message say?

#12 Updated by Erin Clark almost 3 years ago

  • Status changed from Screened to 15

#13 Updated by Andrew Walker almost 3 years ago

Erin Clark wrote:

Should it be always not checked even for new shares? Also, what specifically should the warning message say?

I don't think it should be checked by default. Even for new shares. Sometimes users decide to temporarily expose an existing path via a new share. We don't want them to accidentally clobber permissions by default.

#14 Updated by Dru Lavigne almost 3 years ago

  • Status changed from 15 to Screened

#15 Updated by Erin Clark over 2 years ago

  • Assignee changed from Erin Clark to Lola Yang

#16 Updated by Erin Clark over 2 years ago

  • Status changed from Screened to Needs Developer Review

I have done everything on the gui side, make the checkbox off by default and add a warning message about resetting the permissions, this should always save a value of false unless the user checks the box but the value in the db will still be on until the user saves again so there may be some desire to have a middleware portion that sets the value to 0 after the permissions have been changed

#17 Updated by Erin Clark over 2 years ago

  • Related to Bug #27881: Set the default setting for the SMB -> Sharing -> SMB -> Apply Default Permissions checkbox to checked added

#18 Updated by Lola Yang over 2 years ago

  • Status changed from Needs Developer Review to Reviewed by Developer
  • Assignee changed from Lola Yang to Erin Clark

LGTM

#19 Updated by Dru Lavigne over 2 years ago

  • Project changed from TrueNAS to FreeNAS
  • Category changed from GUI to GUI (new)
  • Status changed from Reviewed by Developer to Resolved
  • Target version changed from 11.2-BETA1 to Master - FreeNAS Nightlies
  • Hide from ChangeLog deleted (No)
  • Support Department Priority deleted (0)
  • Needs Doc changed from Yes to No
  • Needs Merging changed from Yes to No

#20 Updated by Timur Bakeyev over 2 years ago

  • Status changed from Resolved to In Progress

Erin, sorry for reopening this ticket, it's my fault that associated ticket was delayed for so long, but now the REST API has been modified not to store cifs_default_permissions in the configuration DB, it should just launch `winacl` in case it is set to True, so you may want to check your code to coop with that.

Also, according to John's input, we want the corresponding checkbox to be checked when we create a new resource and UN-checked when we edit this existing resource. I've implemented that in the Django, hope should be easy enough for the new GUI as well.

Code is in the master if you need it for testing.

#21 Updated by Dru Lavigne over 2 years ago

  • Target version changed from Master - FreeNAS Nightlies to 11.2-BETA1

#22 Updated by Erin Clark over 2 years ago

So it should be checked when creating one now? There's no danger of messing up the acls of datasets that are underneath the dataset if a share is created using a dataset with child datasets that already have acls set?

#23 Updated by Timur Bakeyev over 2 years ago

If you put it this way - sure, there is a risk :( I'm not sure, does the winacl tracks down the dataset boundaries.

Well, maybe we'd ask John what does he think about such risks and how we can mitigate them.

#24 Updated by Andrew Walker over 2 years ago

My $0.02 on the matter is that applying default permissions is a potentially destructive action and should therefore not happen by default.

#25 Updated by Erin Clark over 2 years ago

  • Status changed from In Progress to Done
  • Severity set to New

This should be good still, the last time I made changes to this I basically ignored the database value and had it default set to false on incoming data. Given how scary and destructive this action can be I am going to leave it unchecked by default on new shares as per the original ticket unless I hear a strong argument otherwise.

#26 Updated by Dru Lavigne over 2 years ago

  • Target version changed from 11.2-BETA1 to Master - FreeNAS Nightlies

#27 Updated by John Hixson over 2 years ago

Erin Clark wrote:

This should be good still, the last time I made changes to this I basically ignored the database value and had it default set to false on incoming data. Given how scary and destructive this action can be I am going to leave it unchecked by default on new shares as per the original ticket unless I hear a strong argument otherwise.

Changing the behavior of how it previously worked is a bad idea. The general use case here is new share with new dataset, not new share with existing dataset. New datasets absolutely need to have good defaults which is what this does. Anything other than this and you are making the user have to do more work by going to the storage manager and doing permissions management. We should have good defaults by default ;-) By removing this, when a user creates a new share and doesn't go to the storage manager, they will not be able to access their share once created, and in turn, many more tickets will be created. Trust me, this used to happen all the time, which is why this "feature" came to be in the first place. I understand it can cause undesired behavior for the case of an existing dataset with data, which is why I have stated in another ticket related to this how this should work. I'll cut & paste that here so it's clear:

1. On new shares, we need to set the permissions the way they currently get set (which this button does).
2. Many users need the ability the reset permissions at all.

I don't have a problem with removing the button, but we need (1) to happen when a new share is created without users having to do any extra work. (2) should be available in this dialog somewhere, but I leave that to the UI experts.

We want to make things as simple as possible for our users, not more difficult ;-)

#28 Updated by Dru Lavigne over 2 years ago

  • Status changed from Done to Unscreened
  • Target version changed from Master - FreeNAS Nightlies to 11.2-RC2

#29 Updated by Erin Clark over 2 years ago

  • Related to Bug #33084: The "Default Permissions" box is not checked when a new SMB share is created added

#30 Updated by Erin Clark over 2 years ago

  • Status changed from Unscreened to Done

I have a PR for this for another ticket, see https://redmine.ixsystems.com/issues/33084, https://github.com/freenas/webui/pull/707

Closing this old ticket

#31 Updated by Dru Lavigne over 2 years ago

  • Target version changed from 11.2-RC2 to Master - FreeNAS Nightlies

#32 Updated by Dru Lavigne about 2 years ago

  • Related to Bug #41784: Default permissions button gets unticked added

Also available in: Atom PDF