-
Notifications
You must be signed in to change notification settings - Fork 14
Do not block access for ALL users when the license user limit is exceeded #192 #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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
| } | ||
|
|
||
| @Override | ||
| public License get(String extensionId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was moved here from Active Directory, in order to reduce class complexity on AD.
| * @param context XWiki context, to help in querying pages | ||
| * @return a reference to the user page with the XWiki.XWikiUsers object, or null if not existent | ||
| */ | ||
| DocumentReference getUserDocFromUsername(String username, XWikiContext context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for this method, all other methods in this interface are only 'nice to have'. They might be useful in the future but will likely need to be modified to fit a real requirement. They are not used in the Active Directory issue, so they can be removed.
In fact, this entire class could be removed from the licensor, unless there is a good reason to have a common interface for paid Authentication Extensions (like maybe altering the Admin section User list UI to show which users are licensed).
WDYT?
| super(HINT, | ||
| Arrays.asList(new DocumentCreatedEvent(), new DocumentUpdatedEvent(), new DocumentDeletedEvent())); | ||
| super(HINT, Arrays.asList(new DocumentCreatedEvent(XWIKI_SPACE_FILTER), | ||
| new DocumentUpdatedEvent(XWIKI_SPACE_FILTER), new DocumentDeletedEvent(XWIKI_SPACE_FILTER))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since valid users are only in the XWiki space, this filter might improve performance. But this change is not integral to this PR.
| @Singleton | ||
| public class UserCounter | ||
| { | ||
| protected static final String BASE_USER_QUERY = ", BaseObject as obj, IntegerProperty as prop " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protected only for being accessible from the UserCounterTest class.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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