It would be very nice to call hook_perm in filefield_sources.module and invoke 'perm' of all sources/*.inc. We will use a custom *.inc (thanks to attach.inc) to enable FTP uploaded files - but would not like all editors to access this functionality. I think, this would be useful for others as well ... so I attach a patch.

I hope you can include it in your dev version as well.
Thanks for the creation of this module, by the way :)

Comments

greg.harvey’s picture

Status: Active » Needs review
StatusFileSize
new3.23 KB

I've gone a step further and added the necessary hooks and logic to some of the sources. New patch attached. Note, no hook required for the IMCE include, because it already uses IMCE permissions correctly, so access to the module can be controlled there.

quicksketch’s picture

Status: Needs review » Postponed

I'm not sure I want to include this. Generally speaking, FileField Sources is just a short cut. A user could easily find a file, download it, then re-upload it no matter what their permissions. Why would you want to make things difficult for some of your users? Now that you can disable/enable certain sources per-field, this seems unnecessary. Unless convinced otherwise, this will not be added.

greg.harvey’s picture

Title: Implementation of hook_perm » Add some ability to hide parts of the UI that are not required by/actually confusing for certain roles
Status: Postponed » Needs work

Actually, you misinterpret what we're trying to achieve here: it's not a case of "making it difficult for some users" - in my case it's responding to a client request to make it *easier* for some users. I can't find the actual email from my client that spawned this patch, but it was something along the lines of:

Can we make it so the journalists only see the Reference existing option? File browser and Remote URL are confusing the little darlings!

Trouble is, this client wanted users to be able to reference existing CCK filefield uploads but wasn't interested in the other options. So, as you can tell, it's not about access control and making things difficult - it's about tidying up the UI and the ability to hide elements some users a) don't need and b) were finding confusing.

It so happens hook_perm() was the quickest win to achieve this, but if you'd prefer it as a separate admin interface instead, because you think going the "permissions" route might give site admins the wrong idea/isn't the right way, then I take your point.

Now you understand the use case, can we develop this patch and include the functionality, even if it's not done with hook_perm()? I think the feature is valid and it can't be an unusual situation I'm in? Not everyone wants/understands the Remote URL option and I just need a way to hide it (and any other additional UI elements that may be added in the future).

quicksketch’s picture

You can now disable certain features globally now that we have the Configuration options patch in (in the 1.0 version), so you can disable the options you don't want. While I see your point, it still seems a little strange that a user could figure out the reference existing but not the remote URL. The file browser I admit needs some work, but I would recommend just disabling it globally if it's too confusing.

greg.harvey’s picture

File browser you can disable by role with IMCE, so I didn't worry about that. We didn't want the jouno users using IMCE anyway.

But it wasn't so much that they "got" one option more than the other. To be honest, they didn't get either! I had to make a nice View of referenceable files with some help text like "copy and paste this line, EXACTLY as it is, in to the online photo reference box", and perfectly formatted the field output so it was what your module expects. They needed leading by the hand anyway, as many users would.

Explaining Remote URL was a waste of time, as some of them would never remember what it's for and none of them would ever use it either, so easier to hide it so you don't have to worry about explaining to them they can just ignore it. Also the client got fed up of saying to his journalists "don't worry about that, you don't need it..." over and over.

I suspect a large part of this is because a lot of these journalists are not young people - many are in their 60s or older. Some don't get on that well with technology anyway and would rather have their typewriters back! So any additional piece of UI is an additional "problem" for them, even if it's just a case of remembering they should ignore it!

On the other hand, the editorial team want all the options, because they're technology literate and trained and it might be useful for them to be able to use a Remote URL (albeit very rarely, it's a feature the client would like to keep as an option for them). We just don't want to show the option to journalists.

So, in short... globally is ok and an improvement, by role is even better. ;-)

If you like, I can do a new patch expanding the global features to have role selectors?

joelstein’s picture

If your feature never gets into the module, you could at least hide the options via some custom Javascript or CSS.

quicksketch’s picture

Title: Add some ability to hide parts of the UI that are not required by/actually confusing for certain roles » Add access control/permissions by role
Remon’s picture

Any chances of this going into the module?

Remon’s picture

Status: Needs work » Reviewed & tested by the community

+1
I find this one extremely useful. In my case, I want to enable extra sources just to staff users, since they are irrelevant to normal users. Frankly, it is disturbing to reroll/apply this patch every time we got a new release of filefield_sources.
Please apply this patch to the module :D. Thanks

quicksketch’s picture

Status: Reviewed & tested by the community » Needs work

This patch needs to accommodate for the new "attach" source.

troybthompson’s picture

Component: Code » General

This would also be great for my Drupal 7 installation. I have community photo project site where the public can upload photos of themselves, but I also take a lot of photos of people myself at events, and want my staff to be able to populate nodes with them. So it'd be great if I could FTP all the files to the server for them to process and select them using your module, but I wouldn't want the public to be able to see that option. Has anyone made a Drupal 7 patch for this?

Morn’s picture

Version: 6.x-1.x-dev » 7.x-1.7

I don't know if it is the right way but it works for me:

added to filefield_sources.module

+/**
+ * Implementation of hook_permission(). 
+ * user_access() was added to filefield_sources_element_info()
+ */
+function filefield_sources_permission() {
+  return array(
+    'use filefield sources' => array(
+      'title' => t('Use filefield sources'), 
 +     'description' => t('Allow users to access this module'),
+    ), );
+}

and modified filefield_sources_element_info():

/**
 * Implements hook_element_info().
 */
function filefield_sources_element_info() {
+if (user_access('use filefield sources')) {
  $elements = array();

  ....

  return $elements;
+  }
}
nublaii’s picture

Issue summary: View changes

I believe this should go in the module by default, as controlling the sources from an editorial point of view is very important.

On our site we allow users to upload videos. We also do some automatic video uploading via ftp/rsync/whatever. I want the editors to be able to attach those files, but I don't want my regular users to be able to see what is being uploaded via ftp/rsync/whatever...

BTW, #12 your code works fine for me ;)

rv0’s picture

Version: 7.x-1.7 » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new1.42 KB
new8 KB

I think the best approach would be to provide an alter hook. That way any module can alter the available sources however they like.
This also keeps the module code clean.

Attached you find a patch that does this and updates the api file with an example implementation. Both the $sources and the $enabled_sources arrays can be modified.
I also include a proof of concept module (rename the .tar.gz), which I might turn into a sandbox or real module later on if more people are interested..
The proof of concept module automatically adds a permission for every filefield source.

In any case it would not harm adding an alter hook for this..

btw, @module maintainers
The default branch is set to "master", should be 7.x-1.x

rv0’s picture

Created sandbox module for my filefield_sources_access module https://www.drupal.org/sandbox/rv0/2426027
requires the above patch in #14 alter_sources-661198-14.patch

ludo.r’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for that contribution rv0.

It works smoothely (both filefield_sources patch + filefield_sources_access module), and the code is simple as hell! :)

damienmckenna’s picture

  • profak committed 2184a8a on 7.x-1.x
    Issue #661198 by rv0, profak: Add access control/permissions by role.
    
profak’s picture

Status: Reviewed & tested by the community » Fixed

Hello @rv0!

According with documentation to drupal_alter:

A maximum of 2 alterable arguments is supported (a third is supported for legacy reasons, but should not be used in new code). In case more arguments need to be passed and alterable, modules provide additional variables assigned by reference in the last $context argument..

So, i put required variables into single context like that:

$context = array(
    'enabled_sources' => &$enabled_sources,
    'element'         => $element,
    'form_state'      => $form_state,
  );

  // Allow other modules to alter the sources.
  drupal_alter('filefield_sources_sources', $sources, $context);

and tested attached module - everything is working. Documentation updated.
Pushed to dev - thank you!

rv0’s picture

Great, thanks for the head up there, I'll have a look at the sandbox again.

profak’s picture

Status: Fixed » Closed (fixed)

In 7.x-1.10.