EZP-29559: Delete sleeping requirement for non-empty roles

[glye]

Question	Answer
JIRA issue	EZP-29559
Bug/Improvement	no
New feature	no
Target version	6.x/7.x
BC breaks	no
Tests pass	yes
Doc needed	no

The role draft should have at least one policy. Uncomment the sleeping code for this, and fix breaking tests.

API team decided this is not worth it - so much code has been written not taking this into account, will require big rewrites without a big benefit, so better to axe it.

[gggeek]

Without reading the code, I have a question: why would we enforce roles to have N >= 1 policies? As long as there is any API that allows checking for a role assignment (instead of directly a policy assignment), I can see some (admittedly limited) value in being able to create empty roles...

[glye]

@gggeek Hi! I don't know the original intention for it, and I can also see that empty roles might be useful to someone, as an easy way of "marking" a user, e.g. to be tested in custom code: "If user has role 'snafu', do X".

But IMO this would be abusing the system. By design, the purpose of a role is to contain policies, and be assignable. It's easy enough to add a custom policy, to fullfill the need above ("if user has policy 'snafu', do X"). And this has the benefit of being easier to audit for someone not familiar with the custom code.

[glye]

@alongosz @andrerom With this fix, empty roles cannot be published. This breaks a number of tests (most fixed in PR now). It also breaks the REST API, in that POST /api/ezp/v2/user/roles becomes invalid unless you add ?publish=false to it. So testCreateRole() here breaks, while testCreateRoleWithDraft() still works.

How do you feel about this? We could make ?publish=false implied, so you'd always get RoleDraft+json back. Expecting Role+json would be invalid. It's a BC break and should strictly speaking go into 2.5/3.0 only.

Or we could drop the PR and remove the commented code instead, accepting empty roles.

[gggeek]

@glye this is all a bit philosophical, as, apart from broken tests, there is no clear advantage presented so far for either allowing or disallowing empty roles - my claim is thus unsubstantiated as has to be taken as is...
It is: given my experience, I have come to the conclusion that adding unnecessary constraints is not a good idea in general, as they sometimes become a hindrance in the future and break or complicate corner use-cases, or the introduction of more specific, useful constraints.
It also is a good idea to keep the data structures, and code, closely aligned to mathematical models, as this helps reasoning about the concepts. And an empty set is a well known, acceptable concept (fe. I would be in favour of allowing Contents without fields, too, if it was not for the need of having a field used to derive the content Name)

[glye]

@gggeek I wouldn't have done this if the code wasn't already there. It was clearly the original intention that this constraint should be in place. But since then, a lot of tests have been written without keeping this in mind, making this PR bigger than I expected. I'll leave it up to gods of API to decide what is kosher/halal here.

As for the use case for empty roles I mentioned, policies are clearly (IMO) better. Role names are (supposed to be / will eventually be) translatable, and they have no identifier, only an ID. Policy module/function strings are untranslatable, and more suitable for identifying permissions. So the constraint arguably inspires better code. I have no opinion beyond that, and there may be cases I haven't thought of.

[alongosz]

This is not going to break just REST API. AdminUI flow uses the same concept:

1. Create role (not draft).
2. Display UI to add policies.

So this would apply to 3.0 (BC break, so needs to be done in major if we agree to do it)
IMHO empty role adds some flexibility, but your point @glye is also valid.

I think this is up to @SylvainGuittard or @bdunogier - should we disallow Roles without Policies in 3.0 and if so - how should we redesign AdminUI.

[glye]

After API team discussion I have axed the sleeping code instead. The big changes that are required are not worth it. Note that the docblock included the exception we never got around to throw, so I deleted that too :)