How to separate actions from security checks?

Question

In my application, the user can perform many different actions, for example. (examples of books / libraries just to make them clearer, the actions in my application are much more complicated):

• Book rental
• Read a book
• To write a book
• Move book
• Book search
• ...

My app also contains some security checks that prevent some of these actions. These checks are not directly related to actions, but represent concepts separate from actions, for example:

• Allowed to open book or not
• Allowed to make changes or not
• Allowed to see the existence of the book or not

Currently, various activities (rent, read, ...) contain code similar to this:

void readBook (Book &book) { if (!checkSecurity (book, OPENBOOK|SEEBOOK)) return; ... } void writeBook (Book &book) { if (!checkSecurity (book, MODIFYBOOK)) return; ... } 

This approach makes it difficult to add new types of security checks dynamically. For instance. dynamic plugin can add additional security checks.

I am currently working on the fact that different actions invoke an interface that looks like this:

 class ISecurityChecker { public: bool isReadBookAllowed() const = 0; bool isWriteBookAllowed() const = 0; ... }; 

The component class implements this interface. Other implementations of this interface can be added to the composite. In this way, the plugin or the dynamic part of the application can simply add new security checks if necessary.

There are some drawbacks to this approach. The main problem is that the interface is getting very large. I'm going to see if I can combine some of the interface methods (because they basically do the same thing or a similar action), but I'm not sure if this will be possible.

Any other alternatives to improve outcome? Or suggestions for improving this design?

Answer 1

There are two possible changes that I can suggest ... and one comment.

First , put your primitive checks in the chain as far as possible. Throw exceptions to report authorization errors.

 void readBook (Book &book) { // readbook doesn't need to perform any checks of it own. // the more primitive actions will scream and yell if the user // isn't allowed to perform sub-action X. openBook(book); BookData d = getBookData(book); // etc. } void openBook (Book &book) { if (!checkSecurity (book, OPENBOOK)) throw new SecurityException("User X is not allowed to open this book!"); // etc. } BookData getBookData (Book &book) { if (!checkSecurity (book, SEEBOOK)) throw new SecurityException("User X is not allowed to read this book data!"); // etc. } 

Second , compare your security actions with actual actions. You can even do this in the data if you want. For instance...

 class Security { // this check get tricky. // if an "real action" isn't listed anywhere, does the user have implicit permission? // (i'm assuming not, for this example.) public static Check(String realAction, Boolean requireAll = true) { Int32 required = 0; Int32 userHas = 0; foreach (KeyValuePair<String, List<String>> pair in Actions) { if (pair.Value.Contains(realAction)) { required++; if (Security.CurrentUser.Actions.Contains(pair.Key)) { userHas++; } } } if (requireAll) { return userHas > 0 && userHas == required; } else { return userHas > 0; } } // hardcoded here, but easily populated from a database or config file public static Dictionary<String, List<String>> Actions { {"OpenBook", new List<String>() { "readBook", "writeBook" }}, {"SeeBook", new List<String>() { "readBook", "writeBook" }} } } void readBook(Book &book) { if (!Security.Check("readBook")) return false; // etc. } 

This Check() method uses the requireAll parameter, but the mappings themselves can just as easily be updated to "insist" or "prefer" to be present for their alleged "real actions".

And my comment . Do not reset your safety. Some safety rules imply other rules that may be meaningless on their own. For example, READBOOK and WRITEBOOK both imply OPENBOOK , and OPENBOOK seems to be pointless. Although OPENBOOK or WRITEBOOK might seem silly to the user without such things as SEEBOOKCOVER and SEEBOOKINSEARCHRESULTS or something else, I would suggest that the only relevant permission when reading a book is READBOOK .

Answer 2

Use the Command-Query Separation (CQS) approach. The general idea is to define a class of commands as follows:

 public class Command { public Command(Action<object> act, Func<object, bool> canExecute) { this.act = act; this.canExecute = canExecute; } private Action<object> act; private Func<object, bool> canExecute; public void Execute() { if (CanExecute()) { act(this); } else { throw new InvalidOperationException("Command cannot be executed"); } } public bool CanExecute() { if (this.canExecute != null) { return this.canExecute(this); } else return true; } } 

Then on the page, you use Command.Execute() instead of the usual book.Write / book.Print , etc.

Using:

 Command readBookCommand = new Command( k => { book.Read(); }, l => { CanReadBook(); } ); readBookCommand.Execute(); 

There may be a better implementation of CQS. This is just a simple illustration.