Refactoring Security Module

Hello everyone,

By this post, I would propose you a refactoring of the security module, motivated by the need to add new code to it without currently having a suitable place to put it.

As of today, the security module is structured as follows :

  • security-api
    • authorization
  • security-authentication
    • authentication-api
    • authentication-default
    • authentication-script
    • authentication-ui
  • security-bridge
    • authorization.internal
  • security-script
    • authorization.script

Taking into consideration the authorization related code, we could take it out of security-api and put it into its own module folder to the same level of authentication and by the way, security-bridge is mostly about authorization too, so it could be put in authorization module.

Also, current authorization module includes more generic classes that could be extracted from this module to create a new security module which could be named security-access or security-common and this one could include all SecurityReference, SecurityRight, SecurityAccess, SecurityEntry and SecurityRule related code. I would also place RightUpdatedEvent and SecurityCache into it.

Eventually, the code I want to add, new and thus configuration, could be added to this module and this module will eventually fit future security related code that may not fit into an other security module.

At last, the security-script could be splitted in the way the top level SecurityScriptService in the newly proposed security-access module and each nested script should be placed into corresponding modules. I don’t know yet if nested scripts should be placed in authorization-api for example or in authorization-script.

Hence, the proposed refactored security module will look like:

  • security-access (this name is open for debate)
    • SecurityReference, SecurityRight, SecurityAccess, SecurityEntry, SecurityRule
    • other security related code that doesn’t need its own module
  • security-authorization
    • authorization-api
    • authorization-bridge
    • authorization-script
  • security-authentication
    • authentication-api
    • authentication-default
    • authentication-script
    • authentication-ui

That said, I’m looking forward your opinion about this refactoring and the name of the generic security module but, we certainly need to refactor it anyway.

Thank you!

I don’t see any reason to rename security-api, it’s already clear from its name that it related to the whole security domain and that’s the naming we use in such a case everywhere else.

If SecurityScriptService is moved to security-api then AuthenticationScriptService and SecurityAuthorizationScriptService should follow the same logic and be moved to their respective *-api modules instead of dedicated *-script ones.

I’m struggling to see the distinction between security-access and security-authorization: aren’t they related to the same need, i.e. to handle rights? Not sure to see why you need this module.

Following a conversation with @vmassol, it appeared that security-authorization may need to be taken out of security-api in order to create a package for it. And despite security related code concerns more or less always rights management, we saw the opportunity to clarify a bit the security module organization and keep only in security-api the generic code not directly related to security-authorization. Is it clearer ?

By the way, as @tmortagne said, maybe keep security-api as the name for the generic thing is the right thing to do.

  • +1 to introduce security-authorization and the nested modules
  • +1 to keep security-api(even though it’s a bit misleading since the other -api modules are also about security and it would probably be more logical to make it security-common-api or security-generic-api or… But security-api is acceptable to me).
  • +1 to either have 3 -script modules or to move the script services into the -api modules. However the later may not be possible if the script services depend on the implementation modules (like security-bridge), and they probably do.

That’s good.

I think we should have 3 -script modules, it’s more coherent with the refactoring .

Ok with that.

Well not sure I agree with that, you could have security code related to authentication (which is the case) or related to cleaning some content: those are not related to rights.

Well you’re saying here that the code is not directly related to security-authorization but in your first post you mentioned:

Aren’t those directly related to authorization ?

So all in all I’m +1 for the changes to have a clear separation between security-authorization and security-authentication with proper hierarchies for both, but I still don’t really see the need on long term for a generic security-api module, so -0 for this one.

It appeared to me, it was related to authorization but kind of generic too. That said, I maybe misunderstood the thing and in fact the whole current api module has to be restructured and renamed with authorization.

security-api module provides a location in which place code that is too little to be considered as a module on the same level as authorization or authentication. For example, XWIKI-18600 fix could be placed in security-api. The alternative is to remove completely security-api and keep each feature in its own security module as authorization and authentication.

So 2 questions:

  • Is the current security-api module in its entirety the upcoming security-authorization?
  • Do you think we should remove security-api and keep creating module for each feature without considering its size?

For me it’s more generic security-related code and also code that other security module depend on (or can depend on).

I don’t know about this but I wonder why some java classes were not put in the authorization package. Like UserSecurityReference. A mistake?

BTW just checked and indeed xwiki-platform-security-authentication-api doesn’t depend on xwiki-platform-security-api which seems to mean that xwiki-platform-security-api doesn’t contain generic security classes ATM.

This seems to indicate we currently should have only 2 modules under xwiki-platform-security:

  • xwiki-platform-security-authorization
  • xwiki-platform-security-authentication

Now security-api still makes sense to me (the name is up to discussion, see above) and is where we’d put security code that is generic. I’d put the SecurityConfiguration class in that module.

This seems ok to me.

  • +1 on 2 modules authentication and authorization
  • +1 on security-api which contains SecurityConfiguration

But where do I put XWIKI-18600 fix ? On the same level as authentication and authorization ? Since it does not concerns either of them (only query made from vm) and according to above choices, it should constitute a module itself.

This is the SecurityConfiguration class…

I get it this time :sweat_smile: