Do not block access for ALL users when the license user limit is exceeded #192#207
Do not block access for ALL users when the license user limit is exceeded #192#207KebabRonin wants to merge 28 commits intoxwikisas:masterfrom
Conversation
…eded xwikisas#192 * Add user check when limit is exceeded
…eded xwikisas#192 * Add user list caching * WIP Tests
…eded xwikisas#192 * Update solr query parameters
…eded xwikisas#192 * Add guest bypass * Make user counter cache update more specific to users in the XWiki space
…eded xwikisas#192 * Remove solr dependency
…eded xwikisas#192 * [Dev checkpoint] Fix for Active Directory issue
…eded xwikisas#192 * Cache optimisation
…eded xwikisas#192 * Cache optimisation
How to store the users managed by an Auth extension (like AD)?The idea for this fix would be to add a mechanism so once the user limit is exceeded for an extension handling users, newer users are disabled automatically, and re-enabled once the license is upgraded. This should be done so we do not block access to the instance. For this, we need the list of users which are part of an extension, to manage it in different situations. On every save of a page that contains a
The question right now is how should we handle this list of users, how to store / mark them, in order for it to be reliable without impacting performance. We identified 3 ways: SolrThe idea would be to add a custom solr field using In short, for a page containing PRO:
CON:
QUESTIONS
Database Query (HQL)PRO:
CON:
In-Memory cachePRO:
CON:
Implementation detail: |
|
An in-memory cache for all users feels perfectly fine. As an optimization, you could disable the feature (and the cache) when the license is unlimited, which should avoid issues on huge instances. Otherwise, we control what kind of licenses we provide so we can test the memory usage for the license limits we provide. But as usernames shouldn't be more than 100 bytes, even 1 million of them wouldn't consume more than 100MB, which doesn't sound much for an instance of 1 million users. Solr should never fail queries while indexing is running, it just returns the results from the last time commit was called. Since XWiki 16.9.0RC1, you can wait for Solr to index everything that has been submitted to the indexing queue before your call, see SolrIndex#waitReady. Note that we already index all XObject properties, it might already be possible with this to search for users managed by the AD extension without any extra attributes. I would be very careful with automatically enabling users as you need to be careful that you don't enable users that have been intentionally disabled by the admin. Instead, I would rather suggest blocking the login itself if that's possible. I wouldn't worry so much about admins temporarily bypassing restrictions, I would more worry about user experience. When you're starting using the wiki and your first experience is that after a few seconds your user is deactivated/on the next login your user is deactivated and you don't understand why, that's bad. There should be clear and understandable error messages for the affected users. |
# Conflicts: # application-licensing-licensor/application-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java # application-licensing-licensor/application-licensing-licensor-api/src/test/java/com/xwiki/licensing/internal/UserCounterTest.java
…eeded xwikisas#192 * Remove unnecessary changes * Fix merge * Remove licensing events
…eeded xwikisas#192 * Remove unnecessary changes
…eeded xwikisas#192 * Add helper method * Fix logic bug
...licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/DefaultLicenseManager.java
Show resolved
Hide resolved
...ensing-licensor-api/src/main/java/com/xwiki/licensing/internal/AuthExtensionUserManager.java
Outdated
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Outdated
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Show resolved
Hide resolved
| // A set of users on the instance, sorted by creation date. | ||
| private SortedSet<XWikiDocument> cachedSortedUsers; | ||
|
|
||
| // Helper to find users in constant time. |
There was a problem hiding this comment.
Since the cachedSortedUsers is populated from the database, dummy XWikiDocuments can't be used to index into the set directly (equals fails because it checks for a bunch of fields like version, author, etc.).
This is the reason why the map is needed.
…eded xwikisas#192 * Add UserCounter Tests
oanalavinia
left a comment
There was a problem hiding this comment.
I added some comments. Some are just for small things, but there are also questions related to the functionality / architecture. I will continue the review once those are clarified, since some comments might not apply anymore after (I think it's the case now as well)
I had another question related to this functionality. Will this affect other authenticators? For example, if I also have EntraID app installed, what's the behavior?
...sor/application-licensing-licensor-api/src/main/java/com/xwiki/licensing/LicenseManager.java
Outdated
Show resolved
Hide resolved
...ensing-licensor-api/src/main/java/com/xwiki/licensing/internal/AuthExtensionUserManager.java
Outdated
Show resolved
Hide resolved
...ensing-licensor-api/src/main/java/com/xwiki/licensing/internal/AuthExtensionUserManager.java
Outdated
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Outdated
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Show resolved
Hide resolved
...ensing-licensor-api/src/main/java/com/xwiki/licensing/internal/AuthExtensionUserManager.java
Outdated
Show resolved
Hide resolved
|
Installing the OIDC extension with this licensor version works, I didn't see any issues logging in. You can't have both OIDC and AD authenticators active at the same time, but that's expected. OIDC has no license/user limit checks as far as I can tell, so nothing should break, you can still log in with too many users on the instance. |
…api/src/main/java/com/xwiki/licensing/internal/AuthExtensionUserManager.java Co-authored-by: Lavinia Vitel <florean.lavinia@gmail.com>
…eded xwikisas#192 * Apply formatting * Remove AuthExtensionUserManager interface
…eded xwikisas#192 * Update test module for ActiveDirectory PR
...t/application-licensing-test-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Outdated
Show resolved
Hide resolved
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Show resolved
Hide resolved
| private Long cachedUserCount; | ||
|
|
||
| // A set of users on the instance, sorted by creation date. | ||
| private SortedSet<XWikiDocument> cachedSortedUsers; |
There was a problem hiding this comment.
Could you see how we could do some performance tests? We would need to know that this doesn't fail on an instance with a loot of users
There was a problem hiding this comment.
I'm guessing this depends on the JVM allocated memory. Iirc, this comment mentioned that the cache size should be ok (at least proportionally to the instance size).
I'll try to create a test with ~10.000 users on the docker instance, but I'll have to see if the test instance can really support 10k users.
There was a problem hiding this comment.
Would be nice if we could have it. I know there were some script to create multiple users / wikis
This could be done as an extra step after you finish the code part :)
…eded xwikisas#192 * Update since tags, comments, formatting
...plication-licensing-licensor-api/src/main/java/com/xwiki/licensing/internal/UserCounter.java
Outdated
Show resolved
Hide resolved
oanalavinia
left a comment
There was a problem hiding this comment.
This looks ok on my side, let me know if maybe you are not sure of some aspects. Thanks for the work!
Small note that the performance tests are still to be done + there is still xwikisas/application-activedirectory#112 which is related
Mostly API changes to better support this functionality in Auth Extensions.
DefaultLicensor.java,DefaultLicenseManager.java- Some additional APIs which accept extension IDs asStringinstead ofExtensionIdUserCounter.java- Added an in-memory cache of active (non-disabled) users on the instance, storing the users in the order of their creation dateAuthExtensionUserManager.java- Added interface for auth extensions, with mostly unused methods