Notes on New Security Model

I was going to send this as a reply to an email, but I don't think anyone cares too much... everyone seems to just want to celebrate anything and everything new instead of actually reviewing things and considering negative impact versus improvement, whether it helps with outstanding issues or not, etc. So, below are some notes I was going to put into an email reply and instead I'll bury them here.

================================================

When I first saw mention of this stuff I thought "great! there are a number of security issues that have been discussed and hopefully this will help make certain things a lot easier and cleaner!" Then I saw the commits coming a day or two after seeing the first mention of it and started getting worried... now that I've looked at it in more detailed I feel just plain frustrated... so just a warning... this appears to be the first real feedback on this stuff (come on, where is everybody else's detailed feedback, other than Adrian's comments on the confluence page?) and it isn't too pretty...

I tried to bring up a point that I thought would help draw distinctions between the old implementation and the new one, and open an opportunity to discuss it's benefits or in other words the requirements that it handles well that the old stuff did not.

Maybe that's a bad idea and a more direct approach is better, so here are some issues...

1. No collaboration on requirements or design, it is ironic because I've just been writing an article about issues with collaboration and some of it addresses this very point... so this is a great case study for it! These problems have happened many times and result in a lot of stuff being ultimately thrown away, or things broken that were working fine just because someone didn't try to understand the existing stuff before replacing it.

2. There is no attempt to specify requirements, and no attempt to collaborate with others on the requirements, and in fact this new implementation (IMO) does little to improve over previous things, the way strings are laid out is just different... it's not better in any major way I can see, just different; many of the "features" of the new layout described are possible with the old permission structure and conventions.

3. There are no "before and after" scenarios that compare previous ways of doing things to new ways. This is pretty important and should be part of a design effort in order to make it more clear what the design does and how it works, and to make it easy to see if the result is easier or more difficult in the after versus the before. The closest thing to this is the diff for revision 770084. Looking at that I'm not convinced that it IS actually better.

4. This ignores common enterprise permission patterns, ignores the current OFBiz patterns for handling functional versus record level permissions, and doesn't handle record level permissions very well... now there is a funky way to tie in a groovy script through stuff that can't be updated on the fly instead of being able to write a permission service using higher level tools that focus on the business aspect of it instead of requiring you to jump technical hurdles PLUS worry about the business level stuff, and do so in a way that is different from how all other business level stuff is implemented in OFBiz.

5. This doesn't make it any easier to do record level permissions; nowhere in the document does it mention the common pattern of creating a view-entity and querying that with the IDs of the user and whatever record they are trying to do an operation on; the current stuff make you bury the use of that view-entity in code, which kills the idea of making these "business driven"; also it does not handle multi-part keys for the records involved in the record-level permissions.

6. One feature I think is a neat idea is the temporary permissions so that higher level permissions apply to the requirement for different lower level permissions in very specific circumstances. That might be a nice solution to some current issues. However, I think there may be some issues with and we should chat about if this opens up any vulnerabilities, ie brainstorm and be creative about ways to exploit this possible weakness. On a side note: this could be implemented as an extension to the current security stuff as well.

7. Backward compatibility: in your response Andrew you said that it IS backward compatible, but it is NOT since both cannot operate simultaneously. In other words, if we change a permission check (like you've done in the example component) to look for the new permission format, if someone has the old permission format it won't work. That means that to upgrade a production instance you must bring down all app servers, update the code on all app servers before bringing any of them up, update the seed/OOTB data, migrate all current SecuryGroupPermission records that point to old permissions, and then you can start the app servers again and not have inconsistent behavior.

8. This is being discussed in another thread, but I'll just mention it here: it is true there are various issues with current security coding practices and conventions in OFBiz, and one of them (that certain other frameworks do far better) is enforcing the same security in the UI as is enforced in the input processing, and for at least common cases using the SAME code or configuration for both (ie so it is not redundant, inconsistent, etc).

So, how should we resolve these things and create a design that more comprehensively handles security issues?

Also, I would really appreciate feedback from others on security complexities you have run into, especially that are not covered here. I've tried to cover major ones that I'm aware of and have run into a lot, but I recognize very well that I often miss things that collaboration with others around here has thoroughly taken care of!