Break Right#ordinal API and its usage in RightSet and RightMap

Development
,
#1

Hi everyone,

I’m opening this proposal following a bug found by @MichaelHamann in the Right API related to the relatively new capability (since 13.5RC1) to unregister a right.

I started to work on a PR for fixing this bug (in XWIKI-21012: Rights are messed up after unregistering a right/uninstalling the like application by surli · Pull Request #2218 · xwiki/xwiki-platform · GitHub) but we found out that the fix Michael proposed would break RightSet/RightMap API. The problem is that right now every rights gets an index based on a static list named VALUES. This index is limited to 64 entries (which is a limitation for which I opened a ticket btw) and each new registered right gets a new value. The bug is that right now if we unregister a right that wasn’t the last one, every rights registered after it are shifted in the list so their stored index returned by Right#ordinal doesn’t match anymore the index they are actually stored at in this list.

Moreover, RightSet and RightMap are designed to entirely rely on this index: RightSet even use it to perform the storage on a long, which explains the 64 rights limitation.

There’s no easy solution for fixing this:

  • if we put null placeholder in the VALUES list to keep the index we risk to get NPE in various API including the ones using RightSet
  • we could use a custom Right.UNREGISTERED placeholder, but then it means the RightSet would contain several times this right in case of multiple calls of unregister which breaks the contract of the set
  • we could try to shift the values of the right after the one unregistered, but then the RightSet in memory wouldn’t be valid anymore and we risk to obtain an IndexOutOfBoundException as its max size wouldn’t be valid anymore

So it feels like the only way to solve this would be to stop having implementations of RightSet and RightMap entirely based on the index of rights, if we’re aiming to support unregistering of rights.
AFAIK this implementation was made by @dgervalle for solving performance issue, but that was a long time ago, I have no idea if we still need such optimization now.
Changing current implementation would also automatically fix the 64 rights limitation.

Another solution, if we want to keep current design, would be to find a way to mark a right as “inactive” somehow: so we’d keep it in the list, but just disable it when calling unregister. It doesn’t look very difficult, at first sight, but there’s probably a huge amount of places where we’d need to actually check if a right is active or not…

Note that I’d like first to have opinions on what direction we want to take, we can discuss later on which branch to apply the solution knowing that it’s a bug impacting LTS too.

#2

Hi @surli,

Your analysis is perfectly right, and for the processing of access rights, and to also limit the memory footprint, I still believe that this long based right map is useful. This is also why I haven’t implemented right deregistration initially. I wanted also to mention that there is a radical but easy solution: clearing the security cache upon right deregistration. So, basically, we start over with a new map. I don’t have the feeling that right deregistration happen very often, so the impact is probably not that critical, and very similar to changing the preferences on the main wiki.

Hope this helps.

Denis

#3

That’s what we already do, see DefaultAuthorizationManager.java#L222. The problem I see is that these data structures are public API and any extension could use them and store instances of them.

I agree regarding the memory footprint, for wikis with many pages and users this could make a significant difference when increasing the size of the security cache.

So if we say that basically “invalidating” all existing instances is okay, we could indeed just compact the values list every time a right is unregistered and give new numbers to rights.

I’ve just searched GitHub for the use of RightSet and only found api-rights and change request as code outside xwiki-platform using it. @surli do you store any instances of RightSet in change request that would be impacted by this?

We could also send an event when the list of rights is changed such that other code can invalidate data structures accordingly.

#4

That’s also my worry by changing the current implem.

I haven’t check but if I do it’s a mistake I need to fix.

IMO that would be best indeed.

Note that on long term it won’t fix the limitation of 64 rights though.

#5

Following this I’m wondering: @dgervalle is there any specific reason why you made RightSet and RightMap APIs? I mean: the APIs are always taking or returning Set<Right> and Map<Right,?> and not a specific implementation. So it feels like RightSet/RightMap should have been kept internal as it’s optimized implementations and the extensions should never directly use them.

If it’s kept only internal then it’s safe for us to manipulate them as we want and in the future to change their implem if needed.
[Edit: Actually that’s not 100% true: if we have APIs returning them, then it means external extensions could have them stored in memory. We’d need to ensure to never actually return those implementations in public APIs]

#6

Well, the main reason for these to be public API was to ease the customisation of XWiki security and reuse of the security cache. It allows custom reader or settler to be easily implemented with the same optimisation. And, as you have noted, we have anyway some API that return them, and those allowing the customisation of the security moduler could probably not without seirous performance or memory footprint impacts, use an alternative implementation.
On the 64 rights limits, based on the right behavior in XWiki, I doubt that allowing more right from a user point of view is acceptable, you should keep in mind that right needs to be manageable, else this is no more secure. So 64 rights is already a lot of individual rights, I am afraid more will be anyway unmanageable.

#7

I don’t agree with you on that part: it’s true with the current UI for handling rights in XWiki, but there’s no reason to keep it in the future. Right now we already have two different UIs for handling standard rights, and extensions rights. For me it would make sense in the future that we could even have separated UIs to set the rights per extension: e.g. if the blog application needs specific rights I have an admin page for the blog to set the rights there, same for change request application etc.
And each extension could even have nested rights: e.g. you specified the rights globally for the whole extension, or you set individually each one of the rights.

So for me it’s really not an issue if we start having more and more rights in XWiki, on the contrary I think it’s a good thing that extension can declare and use them.

1 Like