Projects tigase _server server-core Issues #1151
BruteForceLockerExtension (and possibly others) settings are not correctly retrieved (#1151)
wojciech.kapcia@tigase.net opened 5 years ago
BruteForceLockerVHostExtension extension = session != null ? session.getDomain().getExtension(BruteForceLockerVHostExtension.class) : null;
return extension == null || extension.isEnabled();

In the following code, even though BruteForceLockerVHostExtension is configured as disabled above extension.isEnabled(); returns default true

wojciech.kapcia@tigase.net commented 4 years ago

@andrzej.wojcik I made a (really short) change, that should fix the problem. From what I gathered, in tigase.vhosts.VHostItemImpl.VHostItemWrapper#refresh, during refreshing non-default item, there would be a call to defaults.getExtension(extClass) which would create new instance of such class, but without any data from the repository, thus it would end-up with instance's default true (hence it wasn't possible to disable it).

I added a check (next to the existing ones that make sure that both item and defaults are present) with the same idea - making sure that all extensions of default vhost are also present (i.e. they should have been initialised during loading of default VHost with correct data from repository instead of class' default).

Could you verify that it fits author's intention?

IMHO, there is still a problem with handling 'defaults' of boolean values and lack of clarity what "unchecked" mean - whether disabled or use default. tri-state value would fit here nicely (unfortunately, DataForms doesn't seem to support it).

Andrzej Wójcik (Tigase) commented 4 years ago

Well, I see what you've tried to do, but I'm not sure that you've got that. Basically, now you check if a set of extensions matches. However, if the extension is missing in default and you as for it (call to getExtension()) it will be automatically created. Moreover, with your change, it may just stop refreshing as default may never initialize it's extensions as they were initialized in getExtension() method, so now the sets of extensions may not match any more.

Right now, I'm not sure if the extension refresh will work correctly.

As for merging defaults, each extension has its own custom logic for that and idea how to handle bool values. In case of brute force locker extension, it was assumed that it should be enabled always if default setting or vhost setting is enabled.

wojciech.kapcia@tigase.net commented 4 years ago

Thank you for your comment. I spend more time on this and made a couple of changes (@andrzej.wojcik please review https://github.com/tigase/tigase-server/pull/32).

In a nutshell:

  • Items are refreshed only sometimes
  • We don't set defaultVHost in tigase.vhosts.VHostJDBCRepository#getItemInstance
  • VHostItems within WrappedVHostItem are not wrapped anymore (there is no point)
  • names were refactored slightly to better reflect their intent.

Especially two first points are important - setting defaultVHost when getting item instance also triggered refreshing of default vhost, but it wasn't yet loaded from the database. On the other hand, actual item should be refreshed only after it's loaded from the database or once default vhost is set (to update it's effective configuration).

wojciech.kapcia@tigase.net commented 4 years ago

There is an NPE when default item hasn't been set and client tries to login.

java.lang.NullPointerException
	at tigase.vhosts.VHostItemImpl$VHostItemWrapper.lambda$getExtension$1(VHostItemImpl.java:1616)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1705)
	at tigase.vhosts.VHostItemImpl$VHostItemWrapper.getExtension(VHostItemImpl.java:1613)
	at tigase.server.xmppclient.ClientTrustManagerFactory.getManager(ClientTrustManagerFactory.java:119)
	at tigase.server.xmppclient.ClientConnectionManager.processCommand(ClientConnectionManager.java:746)
	at tigase.server.xmppclient.ClientConnectionManager.processPacket(ClientConnectionManager.java:129)
	at tigase.server.AbstractMessageReceiver$QueueListener.run(AbstractMessageReceiver.java:1401)

One solution would be:

  1. Load all elements from database, create Items from them;
  2. Search for default element and add it to list of items;
  3. Process/add remaining elements.

Other (quick fix):

diff --git a/src/main/java/tigase/vhosts/VHostJDBCRepository.java b/src/main/java/tigase/vhosts/VHostJDBCRepository.java
index 9ce8b6de1..4a50933d2 100644
--- a/src/main/java/tigase/vhosts/VHostJDBCRepository.java
+++ b/src/main/java/tigase/vhosts/VHostJDBCRepository.java
@@ -150,6 +150,11 @@ public class VHostJDBCRepository
 		} else {
 			this.defaultVHost = defaultVHost;
 		}
+		for (VHostItem it : allItems()) {
+			if (it instanceof VHostItemImpl.VHostItemWrapper && it != this.defaultVHost) {
+				((VHostItemImpl.VHostItemWrapper) it).setDefaultVHost(this.defaultVHost, true);
+			}
+		}
 	}
 
 	@Override
Andrzej Wójcik (Tigase) commented 4 years ago

@wojtek I've pushed different solution to the NPE - basically I've moved creation of default NPE (hardcoded values) to the point where vhosts from database are already loaded. That should work correctly and without any issues as in most cases default VHost will be in the database.

wojciech.kapcia@tigase.net commented 4 years ago

@andrzej.wojcik While if got rid of NPE, it broke configuration after startup. I started Tigase, disabled BruteForceLocker for domain X and default domain and later on when I tried to enable it it was impossible.

I'll crate a test case (most likely with some sort of TestExtension that would be enabled and used only in TTS).


Note: from #helpdeskpr-535 - at the moment of addition and calling refresh() does not update the values because defaultVHost hasn't been set yet.

wojciech.kapcia@tigase.net commented 4 years ago

OK, the issue is finally fixed:

  • VHost is properly configured (updated configuring VHost from command) and change is properly propagated to Wrapper
  • Added VHostRepository unit-test to test everything properly
  • Changed unit-test execution from executing all methods in parallel to executing classes/suites in parallel
  • Improved to-string method for VHost to return correct values…

commit:4302d76869913d9ead99e948ad378ec8c5715ca8 (@andrzej.wojcik feel free to take a look)

issue 1 of 1
Type
Bug
Priority
Normal
Assignee
Version
tigase-server-8.1.0
Spent time
51h 15m
Issue Votes (0)
Watchers (0)
Reference
tigase/_server/server-core#1151
Please wait...
Page is in error, reload to recover