search

hamishcampbell / silverstripe-securefiles

SilverStripe Secure Files Module
http://polemic.net.nz
Other
21 stars 22 forks source link

Altered call to extend('onAccessDenied') to allow message changes. #3

Closed marijnkampf closed 13 years ago

marijnkampf commented 13 years ago

As far as I can tell the current extend('onAccessDenied') doesn't allow for customisation of the failure message displayed to the user. I've altered the call to extend('onAccessDenied') to allow overriding of permission failure message.

Sample code to extend the message:

mysite/code/FileMessage.php <?php class FileMessage extends DataObjectDecorator { function onAccessDenied($body) { return "You have to login before you can access this file. You can create an account for free."; }

mysite/_config.php DataObject::add_extension('File', 'FileMessage');

If user doesn't have permission he/she is shown a custom friendly message at the login screen.

hamishcampbell commented 13 years ago

Hey Marijn, thanks for the pull request.

Two minor things.

  1. Could you internationalise the default text? Only en_US/en_GB lang entries required, but if you can do others that would be great. There might already be an entry, anyway.
  2. A potential issue is that if multiple decorators return messages, they will all be imploded and presented to the user. I'm not sure this is the best strategy. Ideally, the decorater that denied access would also provide the message, however that can't happen with the current architecture. Consider that if one decorator passes and another fails, both messages will appear in the $body array. We could use only the first message in $body[] - which might be cleaner for users, we'd just have to make sure the behaviour was clearly documented.

Your thoughts?

marijnkampf commented 13 years ago
  1. I've updated patch-1 with the string internationalized.
  2. As it's onAccessDenied I don't think it will be an issue with multiple decorators, as they will only be called after it is known the call failed. Using $body[0] will only show the body from the last call, which may add confusion. I'm not sure whether the implode or $body[0] would be the best solution.
marijnkampf commented 13 years ago

Don't know whether I meant to Close the pull request (sorry, still getting to grips with github).

hamishcampbell commented 13 years ago

Heh - no, shouldn't close it (means I can't auto-merge from GitHub), but that's ok I can do it the 'normal way anyway.

Re: "As it's onAccessDenied I don't think it will be an issue with multiple decorators, as they will only be called after it is known the call failed. Using $body[0] will only show the body from the last call, which may add confusion. I'm not sure whether the implode or $body[0] would be the best solution."

My feeling is this is not the right way to get an error message. onAccessDenied is a trigger function for auditing rather than for user interaction. The module is specifically designed to allow multiple permission schemes with precedence going to passes over fails. That means a message per permission scheme isn't appropriate (since you'll get failure messages for passed permissions).

A static method on SecureFileController is probably the best option. That way it is globally configurable to something appropriate per site, but default is a nice string.

I'll cherry-pick the relevant commits above, and add the method.