permissions, and the /audio view

+1 for finer grained permissions.

1) create/edit own audio
2) view audio
3) administer audio

similar to this patch:
http://drupal.org/node/33826

Great ideas!

Something similiar to Farsheeds playlist patch is in order here for finer grained control.

The table view of all files is something I have been thinking about for a while as well. If we had a nice table view of all audio files, we could also use that as the table that lets one select audio files to add to playlists.

i like the ideas but i almost think we should split this up into several different issues: the table view (which in my humb opinion should be part of views support #54881) and some improvements in the permissions module.

can we try to flush out a better idea of how the permissions should work? right now downloading affects all users a per-node basis, should we add settings to:
download
stream
administer audio

fyi, this module can't implement a 'view audio' permisison. if you look at other node types, non e of them do this. to do this right, you need the node_access API so it is better left to specialized modules.

other permissions are doable here.

+1 on finer permission granularity!
Visitors should be able to SEE, LISTEN, DOWNLOAD as dictated by permission settings.

Can't wait to see it happen... Thanks for the good work so far!

I'm very interested in this and could do some work on it. I've been hired to make an MP3 product using the ecommerce modules, and I'd like to base the product on audio nodes. The trick, of course, is to ensure that only users who have paid for the project can download the file. But at the same time, all users can see the node, just not download the audio.

My plan is to introduce a hook, or just piggy-back off hook_nodeapi ($op = 'audio download') to give external modules control over whether audio.module does the file_transfer. This would mean minimal changes to audio.module, while letting other modules develop sophisticated controls.

I was about to write Colin to see if such a patch would be accepted, when I saw this thread.

On a side note, I think eventually this sort of thing should not be handled by modules like audio, but by more sophisticated file handling from drupal. However in the meantime it is a blessing that audio.module has its own file_transfer code, as that makes fine-grained control possible.

Hi Dave,

Farsheed here. I think implementing a third party module that can generate a link to download the file would also work. You could put user-centric permissions per file download, but then if that user purchased the audio product, use hook_link to add a 'download this audio file' link. The link to the file would use the audio module to build the download path. You could also use node_access to set the permissions as well.

This might possibly give you more control over the download permissions while leaving the audio.module's download link to remain as a site-wide permission.

Farsheed, thanks for commenting.

Do I understand you correctly? Audio.module generates its play/download links just as it does today, if the user has permission. Independently, an ecommerce module with awareness of audio.module generates another download link for anyone who has purchased? I like the idea of not having to modify audio.module.

In that case I think the ecommerce module must provide its own file_transfer logic and its own download URL. Because hiding the link is not enough. You have to make sure someone who types the download URL into their browser gets access denied if they have not purchased. So you can't use the audio.module download URL because it doesn't have the logic built in.

That case would also make the audio nodes look slightly different depending on whether the user has permission granted by the audio module (audio modules nice play/download links) versus permission by the ecommerce module (adds a link using hook_links).

You suggest using the node_access hook, but that hook is called only for the module to which the node belongs. So to put the logic outside audio.module it would be hook_nodeapi. (If I understand correctly).

I will bring these issues up on the ecommerce mailing list as well. I'd like to hear thoughts from those folks.

I agree with your general approach. I guess what I was trying to get at was to have the ecommerce module or another module create a direct download/permissions link to the file. I thought maybe the audio module had a function like create_audio_download_link that could be re-used in this other module to make the secure link.

Also for the node_access stuff, I was referring to how some access module's control access to certain things in a module, using node_access('update'). Instead you'd have something like node_access('audio download'). Although this requires changes to the node_access database table. It wouldn't be pretty, I don't think.

I don't have much knowledge of ecommerce stuff, so I'm sure emailing to them will be useful as well.

-F

To be clear, I have no intention of changing the node_access database table. I want users to be able to view the node - they will browse it, add it to their shopping cart, etc. I just want them to be able to download the file if and only if they've made the purchase.

Yogadex,

You've probably already seen this, but have you looked at the 'file' module in the eccomerce package? I think it would be great to make this work with the audio module, and 'file' goes a long way towards filling in most of the functionality that a site that sells MP3's would require. Let me know how it turns out and if I can help.

Colin

Colin, yes I'm aware of the file product. And as of now, intimately familiar with it. I've submitted a patch which allows the audio nodes to become file product nodes. The complete patch is here: http://drupal.org/node/62906. I'm attaching just the changes to audio.module here.

These changes to audio module include two new permissions, 'play audio files' and 'download audio files'. If a user does not have one of these permissions they will not be able to access the files. That change was not strictly necessary for what I was doing, but I thought it was nice.

The changes also cause audio module to invoke several hooks. Currently file.module (with my patch) would be the only implementer of those hooks. But there is nothing to stop other modules from implementing them if they want finer grained control over audio downloads. The hooks let other module keep track of changes to audio files, and allow other modules to grant download permission to users who do not have it already.

Please take a look and let me know what you think.

Couple of thoughts on your patch:
* I like the new permissions but I'd like to see them named 'play audio' and 'download audio' rather than 'play audio files' and 'download audio files'. It'll keep it consistent with the rest of the permissions.
* I don't think audio_ec_file_nodetypes() or the calls to it in audio_file_access() belongs in the audio module. Those should go into your ecommerce module.
* I'd like to see audio_invoke_fileapi() follow the ideas layed out in http://drupal.org/node/55556

also, your editor is indenting with spaces not tabs.

I don't think audio_ec_file_nodetypes() or the calls to it in audio_file_access() belongs in the audio module. Those should go into your ecommerce module.

Those are hooks that communicate with the ecommerce module. They are what makes it possible for the ecommerce module to be aware of what's happening in the audio module, and potentially any other module that manages files. In the case of audio_file_access(), audio is invoking hooks to see if any external modules are granting download permission. In other words, these methods need to be audio.module.

I'd like to see audio_invoke_fileapi() follow the ideas layed out in http://drupal.org/node/55556

This patch was my attempt to do this in a universally applicable way. Background here: http://lists.drupal.org/archives/development/2006-05/msg00384.html

I put the "ec" into the hook names because I realize everyone expects something different from a method called hook_fileapi or hook_file_access.

The audio_invoke_fileapi() method is there so that other file API could be invoked from the same method, since I expect there will be other file apis until a lot of dust settles.

also, your editor is indenting with spaces not tabs.

Funny, every other time I've submitted a patch, people complained that it had tabs instead of spaces. :)

Those are hooks that communicate with the ecommerce module.

I understand what they're doing, what I'm saying is that they don't belong in the audio module. You can create a small module to map the hooks from audio to ecommerce. Call it what ever you want, audio_ecommerce or ecommerce_audio_file, etc.

This patch was my attempt to do this in a universally applicable way. Background here: http://lists.drupal.org/archives/development/2006-05/msg00384.html

I put the "ec" into the hook names because I realize everyone expects something different from a method called hook_fileapi or hook_file_access.

[I read that thread and had replied, but I'm not seeing it in the archive so I'm guessing that I'd sent it from an email account that wasn't subscribed to the list.] I'm all for fixing filehandling but I don't want to commit to your implementation before it's decided upon. That's the reason I suggested using the hook_audio, it'll improve the audio module and let you do what you need to do. I'm working on a patch for the hook_audio that I hope to have up in a bit.

Funny, every other time I've submitted a patch, people complained that it had tabs instead of spaces. :)

hehe, yeah, picked that sentence up by the wrong end.

I pretty much agree with Drewish with regards to the patch from yogadex in comment #12:

* I like the new permissions but I'd like to see them named 'play audio' and 'download audio' rather than 'play audio files' and 'download audio files'. It'll keep it consistent with the rest of the permissions.

* I don't think audio_ec_file_nodetypes() or the calls to it in audio_file_access() belongs in the audio module. Those should go into your ecommerce module.

I think the place for this is in an audio specific module in the ecommerce 'contrib' folder. Maybe we should just make an 'audio' folder in that directory.

Here's what I'm talking about. Implement the hook_audio(), return true or false for the access op.

In audio_allow_download and audio_allow_play, please do not check for FALSE and deny access based on that. It's one of my pet peaves about drupal's file api that any module returning -1 from hook_file_download denies access to system files, even if other modules want to grant access. Yes, I see that you look for true first, then false. But its confusing. Why would you want some third party module to be able to override the 'download audio' and 'play audio' permissions?

The model with nodes is, 1) call hook_access 2) if nothing returned, use node_access table. Some older hook_access functions would return false, causing node_access database table not to be queried. That is a bug. Well behaved hook_access functions return true to grant, but never return false because that simply interferes with the node_access database table. Why invite that?

If this patch gets accepted, I'll build a module in ecommerce that enables the audio modules to become products.

The patch has the same weakness as my earlier patch. It checks audio_allow_download in audio_download, but does not check audio_allow_play in audio_play. (If I read it correctly)

In audio_allow_download and audio_allow_play, please do not check for FALSE and deny access based on that. It's one of my pet peaves about drupal's file api that any module returning -1 from hook_file_download denies access to system files, even if other modules want to grant access. Yes, I see that you look for true first, then false. But its confusing. Why would you want some third party module to be able to override the 'download audio' and 'play audio' permissions?

The model with nodes is, 1) call hook_access 2) if nothing returned, use node_access table. Some older hook_access functions would return false, causing node_access database table not to be queried. That is a bug. Well behaved hook_access functions return true to grant, but never return false because that simply interferes with the node_access database table. Why invite that?

Well I'm not dead set on that code but it seems logical to allow modules to both allow or deny the permission. If you're going to check for one why not check for the other? Unless an implementation of hook_audio returns false, it'll just fall through to the user_access(). In the case you describe of hook_access implementation, well that just sounds like a lack of documentation. Anyone else want to weight in on this?

If this patch gets accepted, I'll build a module in ecommerce that enables the audio modules to become products.

Excellent, some variation of this will go in.

The patch has the same weakness as my earlier patch. It checks audio_allow_download in audio_download, but does not check audio_allow_play in audio_play. (If I read it correctly)

You read it correctly, that was an oversite on my part. I've corrected that it in the attached patch.

woops, forgot the patch

it seems logical to allow modules to both allow or deny the permission. If you're going to check for one why not check for the other? Unless an implementation of hook_audio returns false, it'll just fall through to the user_access().

All it takes is one module implementing hook_audio with this code:
return user_access('my special permission');
and then 'play audio' and 'download audio' permissions no longer have any effect. Really what you want?

The rest of this patch gets my +1. Any ETA on when it might get checked in? Will it be checked into DRUPAL-4-7 or just HEAD?

All it takes is one module implementing hook_audio with this code:
return user_access('my special permission');
and then 'play audio' and 'download audio' permissions no longer have any effect. Really what you want?

well, sure but i could add a module that returns true in all cases and set my module weight to fire before your module and circumvent the permission that way. the whole point of hooks is to allow people to write modules to do things that i can't envision. so, yeah, that flexibility is what i'd want.

if no one has objections, i'll commit it to head and 4.7. i need to rename the audio_allow_download() and audio_allow_play() to _audio_allow_download() and _audio_allow_play(). i'll get a new patch up when i get some time.

This patch looks good to me. Nice!

the whole point of hooks is to allow people to write modules to do things that i can't envision

It's what I can envision that bothers me. "I have 'play audio' permission, but I can't play any. What gives?" "I'm uid==1, but I can't download audio files!"

But I'm done trying to convince. Looking forward to the improved audio.module.

I feel like a fool, but I cannot apply this patch. Just me?

dave@george ~/sites/civiclabs/modules/audio> cvs status audio.module
===================================================================
File: audio.module      Status: Up-to-date

   Working revision:    1.51
   Repository revision: 1.51    /cvs/drupal-contrib/contributions/modules/audio/audio.module,v
   Commit Identifier:   e4e444952a14567
   Sticky Tag:          HEAD (revision: 1.51)
   Sticky Date:         (none)
   Sticky Options:      (none)

dave@george ~/sites/civiclabs/modules/audio> patch < /tmp/audio.module_hook_0.patch
patching file audio.module
Hunk #1 FAILED at 130.
Hunk #2 FAILED at 154.
Hunk #3 FAILED at 214.
Hunk #4 FAILED at 238.
Hunk #5 FAILED at 379.
Hunk #6 FAILED at 425.
Hunk #7 FAILED at 480.
Hunk #8 FAILED at 507.
Hunk #9 FAILED at 1177.
Hunk #10 FAILED at 1186.
Hunk #11 FAILED at 1244.
Hunk #12 FAILED at 1265.
12 out of 12 hunks FAILED -- saving rejects to file audio.module.rej
dave@george ~/sites/civiclabs/modules/audio>

In audio_download and audio_play, where you call drupal_access_denied(), you must call die() or return immediately afterward. Otherwise both an access denied page AND a not found page are shown.

I think its better to return; rather than die();, because return permits the devel module to append its content to the access denied page.

yogadex, good call on that. i just got back into town so when i get a chance i'll roll up a new patch.

i think this should be good to go. please review it.

this is a slightly better patch. it adds calls for download and play so that other modules could do statistics tracking, like this: http://drupal.org/node/63765

Looks good to me. I would be satisfied to see it checked in as is, but I do have comments.

When made into a product, the ecommerce file.module will provide its own download path. If that link is used, it will not increment the download counter. If that's a problem, I suggest you implement hook_audio in the audio_module itself, and do your statistics tracking there. The ecommerce module could module_invoke_all('audio', 'download', $node) itself when it allows a download.

Somewhat related: I notice you increment your counters in audio_download and audio_play before the file_transfer(), but you call hook_audio after. What happens when the transfer is cancelled? I imagine this could happen a lot with audio_play, as the user hits the stop button on any song they don't like. In that case would hook_audio be called?

I see you left in the checks for FALSE in the allow methods. But I'm done talking about that so I wont even mention that its a bad idea.
;)

Somewhat related: I notice you increment your counters in audio_download and audio_play before the file_transfer(), but you call hook_audio after. What happens when the transfer is cancelled? I imagine this could happen a lot with audio_play, as the user hits the stop button on any song they don't like. In that case would hook_audio be called?

actually, if you double check, hook_audio() is called before file_transfer(). also, file_transfer() calls exit() if everything works out so we have to increment the counts before calling it.

i committed the hook but ended up moving the location of the calls to insert and update. my reasoning was that if you wanted to be called after audio_update()/audio_insert()/audio_delete() you can just implement hook_nodeapi(). however, if you want to have access to the node's tags before they're written to the file there's no way to do that. so, i moved the calls to the hook after the file is saved to its final location but before the tags are saved to the file.

yogadex, eyeballing your patch at #62906 it looks like the change won't affect you. if it does just remap those calls from hook_nodeapi() rather than hook_audio().