Created attachment 163480 [details] [review]
prevent user from select files and show why

the archive file contains a patch on gtk+ 2.20.1 -file gtkfilechooserdefault.c and a test program.

the aim of the modifications are:

- preventing users from selecting a file or folder in the files list when the action defined for the filechooser is not compatible. for example it is useless to select a read only file when filechooser is in SAVE mode.

- indicating to the user that some files or folder are not compatible with the action defined for the filechooser. This way users will not be surprised by the modifications above.

I added an icon because trying to only insensitive files or folder is not sufficient -from my point of view-. If DO_NO_SET_INSENSITIVE macro is unset, files or folders will be set insensitive if filechooser action is not compatible.

there are special cases when choices could be even more limited that are not handled in this patch. For example prevent to choose a folder in SAVE mode which is not writable and does not contains writable sub dir.

this patch has not been tested on windows.It doesn't use UNIX_MODE attribute but rather ACCESSS_CAN_(READ|WRITE|EXECUTE) attributes. So this should be portable.

br

The solution for the "the default selected folder is read-only" scenario could be to simply reset the filechooser to the "recently used" view (that is part of recent releases).

See evolution bug 673097 for why this matters.

Also, could someone review the patch 163480?

*** Bug 137515 has been marked as a duplicate of this bug. ***

Created attachment 231164 [details] [review]
patch from Laurent Wan, extracted from the tarball

From: Laurent Wan <dev.wan free.fr>
Subject: prevent user from select files and show why
Date: 2010-06-12 16:49:19 UTC

---

Unreviewed, but let's get it in an easier form for review than a tarball.

Review of attachment 231164 [details] [review]:

I am not a Gtk reviewer and this is not an "official" review. If it was, I'd be setting the status to needs-work.

This bug is more complicated than I thought it would be :-/

The more I think about it, the more uncomfortable I am with with second-guessing the operating system, particularly when our implementation is in terms of access() (known to be imperfect at best).

Sometimes (e.g. network file systems, Linux setfsuid(), POSIX ACLs, Windows ACLs) the only reasonable way to find out whether something will work is to try it; if we make it impossible to select the appropriate file, then we remove the ability to work around imperfect filesystems, and prevent the user from doing things they would in fact have been able to do.

::: gtk/gtkfilechooserdefault.c
@@ +226,3 @@
+	PANGO_TYPE_ELLIPSIZE_MODE, /* MODEL_COL_ELLIPSIZE */		\
+	G_TYPE_BOOLEAN,            /* MODEL_COL_SELECTION_ALLOWED */	\
+	GDK_TYPE_PIXBUF

This deserves a comment /* MODEL_COL_SELECTION_ALLOWED_PIXBUF */ probably.

In newer Gtk, there is a MODEL_COL_IS_SENSITIVE already.

@@ +4496,3 @@
   gtk_tree_view_column_set_title (impl->list_name_column, _("Name"));
 
+  /* icon form selection compatibility */

After a few attempts, I parsed this as a typo of "icon for selection compatibility", i.e. whether the file's permissions are compatible with the selection mode we're using.

Perhaps "Icon indicating that permissions are incompatible with the action"?

@@ +4498,3 @@
+  /* icon form selection compatibility */
+  renderer = gtk_cell_renderer_pixbuf_new ();
+  /* We set a fixed size so that we get an empty slot even if no icons are loaded yet */

Does this reserve space for the icon, even if the file is actually OK? That seems a bit wasteful of space for the common case where the files are your own.

@@ +6679,3 @@
       g_value_set_boolean (value, info == NULL || _gtk_file_info_consider_as_directory (info));
       break;
+    case MODEL_COL_SELECTION_ALLOWED:

I think most of this branch deserves to be a helper function.

@@ +6705,3 @@
+		    break;
+		  case GTK_FILE_CHOOSER_ACTION_SAVE:
+		    val_bool = can_write;

Depending how the application is going to deal with the file, this might not actually be appropriate. If the application does the equivalent of this Python:

    f = open("file.txt", "w")
    f.write("hello, world!")
    f.close()

then it needs write permission on the file but not the directory (and is buggy anyway, because if it crashes or loses power halfway through writing the file, the file might be in an intermediate half-written state - data loss).

If it does the equivalent of this, like g_file_set_contents() does:

    f = open("file.txt.tmp", "w")
    f.write("hello, world!")
    f.close()
    os.rename("file.txt.tmp", "file.txt")

then it needs write permission on the *directory*, but not the file.

I think if anything, we should assume the latter (i.e. assume a non-buggy application).

@@ +6712,3 @@
+		  case GTK_FILE_CHOOSER_ACTION_CREATE_FOLDER:
+		    val_bool = FALSE;
+		    break;

In more recent Gtk, we call _gtk_file_info_consider_as_directory(), which treats mountable locations and Windows shortcuts as being "enough like a directory". This should, too.

@@ +6720,3 @@
+	      case G_FILE_TYPE_DIRECTORY:
+		//if can_read not possible file list will not be available but that's all
+		//maybe we could create false line with message to avoid popup.

Gtk is approximately C89; C++/C99 "//" comments are not supported.

Maybe we could show a GtkInfoBar "You do not have permission to list files in this directory", or something, when in an executable-but-unreadable directory?

@@ +6724,3 @@
+		  {
+		  case GTK_FILE_CHOOSER_ACTION_OPEN:
+		    val_bool = can_execute;

This is more subtle than it looks. Is access::can-execute exactly the Unix +x bit - which would mean emulating it on Windows - or is it about executability of binaries/scripts, and hence undefined or platform-specific for directories?

GLocalFileInfo makes it defined in terms of g_access (., X_OK), which in practice will never be true on Windows?

@@ +6732,3 @@
+		    //further enhancement:
+		    //FALSE if there is no subdirectory to select and the current directory is not writable
+		    break;

I'm inclined to agree with the assessment that we should just make all "cd'able" directories sensitive.

I don't think we should get into recursing arbitrarily deep into a directory looking for a writable descendant. Consider this hierarchy:

    /srv
        chroots
             ubuntu
                 var
                     tmp

where the file chooser starts at '/' (either explicitly, or because the user pressed backspace a lot). All levels are owned by root, only the last is writable by ordinary users.

We clearly do want to be able to navigate into this - so srv and chroots can't be insensitive. However, we don't want to have to scan the entire /srv/chroots/ubuntu hierarchy before we decide whether the user can change directory into /srv/chroots.

If we're not doing arbitrary recursion for this check, I see two internally-consistent rules we can follow: either we allow entry in all cases, or we allow entry if the directory either is writable, or has one or more subdirectories. But that seems pretty strange - I don't think it'd be obvious to users why they could enter some directories but not others.

@@ +6748,3 @@
+	      default:
+		val_bool = FALSE;
+		break;

This case is reached if the file is neither a regular file nor a directory - notably, symlinks and Unix "special files" will get here.

When we're in a file-centric mode, we want to allow these. If the directory is writable, g_file_set_contents() can probably replace a special file or symlink with a regular file just as easily as it replaces a regular file with a regular file.

@@ +6816,3 @@
+	    return FALSE;
+
+	  if (!_gtk_file_system_model_get_iter_for_file (model,

This seems a pretty complicated way to go about it. I would be inclined to either call the same helper function (info, flags) -> "can we select it or not?" as is used for the "is it sensitive?" column, or only have that column and use a custom cell renderer to turn a boolean into a pixbuf.

@@ +6821,3 @@
+	    g_assert_not_reached ();
+
+	  gboolean sel_allowed;

Gtk is approximately C89, so variables in the middle of a block - a C++/C99 feature - are not allowed. This will need to be hoisted to the top of the block.

(Notably, MSVC++ in C mode doesn't allow variables in the middle of a block, despite the fact that as a primarily C++ compiler, it obviously has to accept them in C++ mode.)

@@ +6824,3 @@
+	  gtk_tree_model_get (tree_model, &iter, MODEL_COL_SELECTION_ALLOWED, &sel_allowed, -1);
+	  GdkPixbuf *icon = gtk_widget_render_icon (GTK_WIDGET (impl),
+						    sel_allowed ?  GTK_STOCK_APPLY : GTK_STOCK_CANCEL,

APPLY and CANCEL are obviously not the right icons, but they're fine for a prototype.

In Gtk 3, stock icons are no longer a thing. If we have an icon at all, I think we probably want emblem-unreadable or emblem-readonly, or maybe action-unavailable-symbolic?

@@ +10168,3 @@
+				       "pixbuf", MODEL_COL_SELECTION_ALLOWED_PIXBUF,
+				       NULL);
+#if DO_NO_SET_INSENSITIVE 

DO_NOT_SET_INSENSITIVE would be better grammar.

A compile-time choice is fine for prototyping, but if merged, we should make our minds up one way or the other. I would go for the version that makes unsuitable files insensitive, personally.

@@ +10175,3 @@
+#else
+  gtk_tree_view_column_add_attribute (column, renderer, "sensitive", MODEL_COL_SELECTION_ALLOWED);
+#endif

I like that the "set it to be insensitive" branch moves all logic for sensitivity into the SELECTION_ALLOWED column. always_sensitive was a bit mis-named anyway, it really means "may select non-folders".

In this branch of the #if, I think the computation of "always_sensitive" is dead code that can be removed.

In newer Gtk, always_sensitive has disappeared anyway.

@@ +10177,3 @@
+#endif
+
+  walk = walk->next;

Unwrapping this loop over gtk_cell_layout_get_cells() means that this function is sensitive to the number and order of cells, and must be changed whenever new cells are added. I would be inclined to keep the loop.

Going back to the original report:

(In reply to comment #0)
> If it can determine
> that a folder is not writable, indicate that in the folder list and prevent the
> user from selecting it (maybe desensitize it and show a lock emblem or
> something).

I don't think desensitizing the folder is valid, because (in my /srv/chroots/ubuntu/var/tmp example) if we did, you wouldn't be able to navigate through chroots, ubuntu, var to get to the tmp folder, in which you *can* write.

A more realistic example is that you're in /tmp/tempdir.xhg3t8gh/ or something. From here, you should be able to go "up" to /tmp, "up" to /, into /home, and from there into /home/yourname - desensitizing unwritable folders would break the / -> /home step.

> If the currently selected folder is not writable, perhaps show an
> inline warning message and desensitize the Save button.

I would be tempted to say "display a GtkInfoBar but allow Save anyway" - to justify desensitizing the Save button we'd want to know "you definitely can't save here". However, I think the best we can actually expect from things like access(), particularly in the presence of networked filesystems and gvfs, is "I don't think you can save here, but you're welcome to prove me wrong".

(In reply to comment #2)
> The solution for the "the default selected folder is read-only" scenario could
> be to simply reset the filechooser to the "recently used" view (that is
> part of recent releases).

I think this is orthogonal to what Matthew suggested and Laurent implemented? (Laurent's patch doesn't seem to implement it, anyway.)

It's an interesting approach to #673097; it seems to me like an odd special case to have. On the other hand, getting it wrong (access() thinks you can write here but actually you can't, or vice versa) has a less bad failure mode here - if it goes wrong, the current folder isn't what you expected it to be and least-astonishment is violated, but at least you can correct for it by navigating to the right place.

(In reply to comment #6)
> I would be tempted to say "display a GtkInfoBar but allow Save anyway"

One nice thing about this is that we can decouple it from what the operating system allows, giving allowed-but-questionable situations the same warning as disallowed situations. If you have a read-only file in a read/write directory, the operating system will happily let you "overwrite" it via the tempfile-and-rename dance, but you don't necessarily want it to be too easy to do so.

Analogously, if you open a read-only file in vim, it opens it in read-only mode, issues "W10: Warning: Changing a readonly file" when you start editing, and won't write it with the normal ":w" command - but if you use ":w!" ("force write"), it uses tempfile-and-rename to overwrite the file.

(In reply to comment #6)
> I think the best we can actually expect from things like
> access(), particularly in the presence of networked filesystems and gvfs, is "I
> don't think you can save here, but you're welcome to prove me wrong".

On the other hand, Nautilus doesn't allow "Create New Folder", "Create New Document", "Paste" etc. in folders that GIO thinks are read-only; so if a gvfs backend gives incorrect results for access::can-write, it's already considered to be a bug in that backend (e.g. Bug #598206).

Some random observations using Laurent's test program, on Linux:

Searchable but not readable
---------------------------

The scenario of a directory that is searchable (+x) but not readable, but has a readable subdirectory, is not so far-fetched. On a multi-user machine, it can be useful to set home directories to rwxr-x--x, to not leak private information encapsulated in filenames, and provide a secondary layer of security-through-obscurity if files of unknown name are accidentally left readable, while allowing access to "well-known" files and directories like ~/Public/, ~/public_html/, ~/.plan and so on if they are made world-readable.

It possible to navigate downwards through a directory you can execute but not read using the mouse; you get a popup "Could not read the contents of %s". I feel as though this ought to be an infobar, so that it doesn't demand attention? Then you'd be able to click on /home/otherguy, see the infobar without it interrupting you, Ctrl+L "Public" Enter, and be in /home/otherguy/Public as intended.

It is also possible to navigate downwards through a such a directory without getting a popup, by using the keyboard: for instance while in /home, type Ctrl+L "otherguy/Public" Enter. However, you get one critical from Gtk per letter of Public typed:

(a.out:21895): Gtk-CRITICAL **: gtk_tree_model_get_iter_first: assertion `GTK_IS_TREE_MODEL (tree_model)' failed

Once you're in the readable subdirectory, it is possible to navigate upwards to the unreadable directory with Alt+Up or by clicking in the location bar. When you do, you get the same popup.

Readable but not searchable
---------------------------

If a directory is readable but not searchable (+x), GtkFileChooser doesn't give any indication that anything is wrong, except that file modification times are "Unknown", file sizes are 0, and subdirectories don't get an icon. I can't think of any reason why a directory would be readable but not searchable, apart from user error.

In principle, Gtk/GLib ought to be able to get mtimes, sizes and file/directory status from readdir() or equivalent, which only needs +r on the directory - but presumably it's using (l)stat() or equivalent, which does need +x. Trying to do better than stat() seems like wasted effort unless someone has a real use for +r -x permissions, though.

Trying to write
---------------

If a directory is -x and we're trying to write a file or create a folder, it might make sense to make it insensitive, because there's no way we can do that inside that directory. Conversely, if we're trying to select a folder, it should stay sensitive for selection and entry, because that directory itself or one of its immediate descendants might conceivably be the one we want to select - we won't be able to read it ourselves, but we might still be able to, for instance, configure a system service to read from it.

If it's +x but -r or -w, we should still allow traversing through it, because it might have a descendant that's +w (obvious example: go through /var, which we can't write, to get to /var/tmp, which we can).

Because an infobar is less intrusive than a popup, once we have one it might make sense to display "[emblem-unreadable] Directory is read-only" (or something) in the infobar whenever we're in a read-only directory with write intent? In the "navigate from /home/smcv to /var/tmp" use case, it'd appear in /home, /, /var then disappear when we reach /var/tmp.

*** Bug 676568 has been marked as a duplicate of this bug. ***

Has this bug been forgotten about? I see some work was made but apparently never implemented.

See comment 5: "This bug is more complicated than I thought it would be :-/" - anybody is free to pick up existing work.

Hello,

 I'm very happy to see that my patch has contributed a little. Sorry for responding this late, 3 years before  i wanted to help on a subject that seems accessible to me. i waited for the review and unfortunately i missed the review in the flow off gtk+ devel list.

 I will try to take account of Simon remarks in the patch and also port it to the latest gtk+ stable release.

 The patch intention was a sort of prototype to see what could be reasonable to do  in this case. I think that this bug is complicated because there is a lot of case to deal with.

 FileChooser is an interface to the filesystem so user can do a lot of things and this lead to this complexity.

 In command line mode you are supposed to know what you are doing because command line mode is a tool you choose to use. This is not a mandatory interface imposed to you. So cd /var/log/message.txt will lead to an error and that's not a problem and nobody will fill a bug for this.

 File chooser is a way to interact with the filesystem -as in command line mode- but this time it is used by everybody and some people are not supposed to know the subtility of file system design. they will learn but they have to understand the way things works. To understand why something goes wrong you have to notice it. I think filechooser miss a way to display information to the user. The means to display information could be a popup, infobar or maybe something else. Whe have to prototype the logic first.

 Again, very glad to have some feedback.

what about someone with propers rights setting Whiteboard field to "filechooser-design-needed" for https://wiki.gnome.org/GtkFileChooser ?

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new

(In reply to Matthias Clasen from comment #16)
> If this bug is still relevant to you, you can open a new issue describing
> the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

I have a little bit problem with the attitude here. It is not whether the bug is relevant to me, it sounds like you are doing me some kind of favour in fixing the bug in your software. It is not my problem, it is yours problem, that your software is buggy, and you should be grateful to kind reporter for helping you to discover it. And finally, it is your problem, that you left bug laying around for nine years, even though it includes some suggestions for fix.