Show an error message if permission names are not unique

[gvkries]

Currently, permission names are expected to be unique across all modules. This change displays an error message in the Role Editor when a permission name is reused. This alerts the administrator to possible security issues when a module redefines an existing permission.

Fixes #8469

[MikeAlhayek]

@gvkries I don't think throwing an exception is the right approach here. Instead, I would recommend logging a warning and proceeding.

Imagine a scenario where a developer creates a module that exposes a permission named X. Later, in OC, we decide to also expose a permission named X. This would result in a naming conflict that breaks an application that was functioning perfectly fine before.

Since permissions are string-based, such naming conflicts are acceptable to some extent, but they should still trigger a warning to notify developers. However, I don’t think it’s appropriate to throw an exception that would stop the application from running entirely.

[gvkries]

I understand the concern about breaking existing applications, but the security implications of reusing permission names are significant:

1. Security Risk: Reusing permission names can lead to unauthorized access if existing roles already grant the reused permission. This creates a serious vulnerability.

2. High Impact: The potential consequences of such conflicts are severe. Logging a warning might not be enough, as it could be overlooked, allowing the issue to persist.

3. Immediate Action: Throwing an exception ensures the problem is addressed immediately, aligning with the fail-fast principle to catch and fix errors early.

While the current solution only causes the admin portal to fail, it highlights the issue and prompts immediate action. Given the high stakes, enforcing an exception is necessary to maintain application security.

[MikeAlhayek]

I understand the concert. This is something I also mentioned few weeks back in the meeting. But I don't think its a justification for throwing a runtime exception. I think logging it as a warning is enough unless we need to try to use namespace to ensure uniqueness which is also bad since it would increase the cookie size.

[gvkries]

Okay, instead of throwing an exception, I'm content to show an error in the admin portal. I think that the mere logging has too little visibility.

I also admit that the previous solution was not sufficient because throwing an exception in the DefaultPermissionsService and admin portal is not reliable enough. For the same reason, I think that logging there is not good enough as well and I have therefore refrained from doing so for the time being.

[gvkries]

Note: Changing the permission names to include the module name or namespace is not an easy solution. This would be a massive breaking change IMHO.

[MikeAlhayek]

Note: Changing the permission names to include the module name or namespace is not an easy solution. This would be a massive breaking change IMHO.

Yes I agree. I am not suggesting that either. I think we should log warning and not throw an exception.

[gvkries]

@MikeAlhayek Maybe we can briefly talk about this tomorrow.

[MikeAlhayek]

Absolutely