Document how and when to add classes/permissions/packages to the engine module

[keturn]

From a comment in #4622:

I see - I was wondering whether these [engineModules] are subsystems or something like that.

Ah, you may have got that idea from the ModuleManager constructor argument named classesOnClasspathsToAddToEngine, which is handled in loadEngineModule. Those are subsystems:

Terasology/engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java

Lines 184 to 185 in 1e9a36c

	// add all subsystem as engine module part. (needs for ECS classes loaded from external subsystems)
	allSubsystems.stream().map(Object::getClass).forEach(this::addToClassesOnClasspathsToAddToEngine);

…

The lists of allowed packages and classes are used in a straightforward manner, from what I've been able to tell. You list a thing there, and it gets used in a conditional somewhere that does a direct comparison against the list. If it's allowed, then any module is allowed to load that class and do whatever it wants with it.

The things that get passed to a Reflections configuration, on the other hand, are not so direct. Reflections isn't really there to provide permissions checks. Reflections data is used to answer questions of "What classes have this annotation?" and "What are the subtypes of this class?"

– but we want to make sure the answers to those queries don't return things from modules that aren't active.
– and failing to get the answer you expected from one of those things can sure look a whole lot like when a thing is missing from the permission list.

We could use clarity on when a class needs to be

• added to the permission list
• added to classesOnClasspathsToAddToEngine
• be annotated with @gestalt.module.sandbox.API
• be added to the permissionSet independent of any of those things?

  Terasology/engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java

  Lines 186 to 187 in 1e9a36c

  	permissionProviderFactory.getBasePermissionSet().grantPermission("com.google.gson", ReflectPermission.class);
  	permissionProviderFactory.getBasePermissionSet().grantPermission("com.google.gson.internal", ReflectPermission.class);

• be included by the engine module's classPredicate https://github.com/MovingBlocks/gestalt/blob/4b6ea6d50176da0ac0d93a9186f1b44ba2cb12c4/gestalt-module/src/main/java/org/terasology/gestalt/module/Module.java#L60
  classPredicate: Predicate to determine what classes to include from the main classpath (classes from the unloaded classpaths will be included automatically)

the Module.classPredicate being a thing that's either new to v7 or was just well-hidden in v5. You can have a class that the security policy and clasloaders all allow you to load, but something may ask ModuleEnvironment.getModuleProviding about it, and if no modules recognize it then it's not usable for some purposes after all.

oh and also these should probably line up somehow? https://github.com/MovingBlocks/gestalt/blob/4b6ea6d50176da0ac0d93a9186f1b44ba2cb12c4/gestalt-module/src/main/java/org/terasology/gestalt/module/Module.java#L56-L57

• fileSources: Any sources of files that compose the module. Must not be null - can be EmptyFileSource
• classpaths: Any extra classpaths to load for the module

[keturn]

Related: #4639

[keturn]

Some notes from discord:

That's the crux of it. What is in the “engine module”? We made sure to move a lot of stuff in to the org.terasology.engine namespace, which makes it somewhat easier to tell what isn't engine, but that wasn't sufficient because there are still things inside org.terasology.engine that shouldn't be module-accessible, as well as things outside (subsystems, TeraNUI) that should.

Complicated by the fact that it's not only a question of “are you authorized to reference this class." We can make classes accessible without putting them in a module.

The other questions are things like “does this class need to belong to a specific module? e.g. to generate a URI for it“ and ”do we need a ModuleEnvironment to be able to answer queries like getSubtypesOf or getTypesAnnotatedWith for this class?“

[keturn]

Some notes from #4799 as a case study:

• If a class from a third-party library fails to load, but you can reference that class directly from engine code, that class (or the package containing it) needs to be added to the list in ExternalApiWhitelist. These lists only use exact matches, wildcards don't work here.

• If a library fails to read a system property, and that property is not something that needs to be kept secret, you can add read permission for it as done here:

  Terasology/engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java

  Lines 258 to 262 in c80f299

  	// In theory, PropertyPermission has wildcard matching and "reactor.*" should be sufficient to grant read access to all
  	// reactor configuration properties.
  	permissionSet.grantPermission(new PropertyPermission("reactor.*", "read"));
  	// In practice, the permission checks fail unless these are each named explicitly.
  	permissionSet.grantPermission(new PropertyPermission("reactor.bufferSize.x", "read"));

  
  (though if we get many more of those we should refactor them out of that method)

[Spoorthy1423]

Hey!! I would like to work on this issue @keturn , actually i already started working and i hope it fulfills the requirements