Config, Context, and Security

[Cervator]

So we've had some discussion lately on IRC, on issues, on Slack, and so on. It tends to get mixed up into a few related but maybe segregated topics, then gets too far off track or complex to follow easily. Thought I'd try to outline it here in a forum thread and let @manu3d also outline his recent musings and discussion on the topic.

I am definitely guilty of mixing it up myself as I try to link topics wanting to be sure related concerns are considered up front. My primary personal desire is getting the Context piece "finished" and applied so we can better control state in-game and have the world preview, module-specific config, and other things working better

Earlier I had thought there were only two topics and they were linked somehow, but I might have connected them where really just one lives inside the other, yet the two can live in separate harmony conceptually. There is also a third topic that got mixed in which is probably much further out - and might even itself be split into two. They are:

• Simple non-hard coded configuration system - issue #2668 (lots of discussion) and PR #2757. Something @manu3d would like just to make rendering config more flexible to use (would naturally also be useful for other stuff)
• CoreRegistry -> Context. Long-standing desire to avoid the global nature of CoreRegistry in favor of the context-sensitive.. well, Context. @Florian started this, but it is a big thing to apply everywhere
• Inter-module sandboxing. Recently brought up by @SoniEx2 / APNG for perhaps somewhat esoteric reasons, but ultimately something we might want to consider
• Modules with extended permissions - #1703. Sometimes referred to as "Android style module permissions" but previously thought of primarily to sit between engine and modules, not modules to modules.

Configuration

This is the easiest / smallest piece and already has a pending implementation PR. Some related stuff that has come up:

• Splitting implementation from interface. This seems to have architectural support, but I am unsure if it relates here or more in general for the engine. Does the current PR pass that check or could it be done better?
• External library (like typesafe config) - likely lacks willing implementation effort since we're pretty far along with FlexibleConfig. Unsure if it passes the split implementation/interface notion.
• Is any consideration needed at this level for how config would work in different places? In the sense of module-specific config, config only being present in a particular Context, etc.
• Does this end up intruding any on config done in Components, does it need to be more ES friendly somehow? I noted how weirdly some world gen config (which gets displayed in the create game UI) use Component classes seemingly without interaction with the ES. Is that an example of where this kind of config would be a better alternative, or should the two link together closely enough to be seamless between UI config and ES Component state?

I'm not an architect but am hoping this topic can be entirely on its own we can just process the current PR for merging soon. It isn't very large and even has unit tests! Can we just agree on a way to store this inside Contexts, inject it nicely, and all is happy in the world?


CoreRegistry -> Context

This is a much bigger topic. CoreRegistry is in use all over the place (989 hits in my mega-workspace...) and while Context is ready to be used and in use plenty of places too all old usages are not yet converted. I am unsure if anything is outstanding before Context would be able to take over entirely for CoreRegistry. Also unsure how much if any change is expected to be able to fully support whatever a refactored Config system would need. Something about injection strategies?

Digging around I found issue #1770 which is an older issue by @Immortius for some engine improvements including assistance for removing CoreRegistry, but while it has a very hefty looking commit listed I don't think it was ever merged. May not have been finished and at this point it might be stale enough to involve problematic conflicts - although we did go v1.0.0 with the engine just a few months later, after which the related architecture may have been more quiet.

Am very curious on how this item would proceed but I suspect it may be longer term and likely need an engine bump to v2.0.0. I am also unsure who can sink effort hours into this.

Having actually written this I'm a little more relaxed in thinking this at least is more separate from the Config item than I thought, with Context just being a container. We still do need to work on it though. I wonder what if any impact there'll be from gestalt-entity-system progressing?


Inter-module sandboxing

The example that came up in issue #2668 was about having a module manage its own whitelist for which other modules could use it. Conceptually this seems useful, but it may not be very needed yet. The stated example would be the first, and I don't think it is enough to mandate implementation of a probably very serious architecture item in the near-term. Gestalt-module was implemented with the expectation that all modules would be equal and playing in the same sandbox, so changing that would be pretty major.

Probably we should shelve this one and keep it out of the two prior topics. Maybe keep it in mind for design purposes, but it is too far off to mix into anything current without blocking it for months if not longer.

It also would likely tie closely with the item below as some things you might want to keep private in a module is extended permissions granted by the engine. Yet other modules could define their own module-internal goodies they might want to keep away from others.


Modules with extended permissions
Perhaps interestingly as per notes in #1703 this is already supported to some degree in Gestalt, at least for the engine. It just hasn't had any implementation work done engine side (maybe in #1705 by @Marcin Sciesinski?), and as part of that the CoreRegistry -> Context piece should probably be finished first.

The idea is that modules could be allowed access to extended engine features not on the regular API whitelist via user/admin prompt/config. As a potential alternative to this could be engine extensions that do not live inside the module sandbox nor within the engine repo itself. I used to refer to that using a new /extensions directory with the same Gradle functionality as libs, meta repos, etc. That way you could develop the more peripheral engine features away from the engine repo itself, like VR / motion controller stuff or touchscreen support. Some others may call this "unsafe mods" or so on.

Like the previous item this is likely a substantial undertaking to do right, and would block anything in the short to mid term, so this is another item I would delay until at least we have some volunteer effort available. However since it does seem to be supported via gestalt-module somebody could try to run a proof of concept.

Others with potential interest: @msteiger @Skaldarnar @Rostyslav Zatserkovnyi

 

[manu3d]

Thanks Cervator for the write up. I agree pretty much with everything you write, in particular the prioritization of Context and new Config in comparison to the other architectural issues.

For the Context work, I don't quite understand why the Context can only be obtained via injection or through the constructor as recommended by @Flo. It might be perfectly reasonable but it slows the migration to Contexts considerably given that some objects are instantiated multiple levels beneath the last object having a reference to a Context. If it was up to me I'd write something like Context.getInstance() to retrieve the main context and then I'd build a hierarchy of specialized Contexts providing access to their parent Contexts if necessary. I also don't understand why Context's keys are classes. Why can't they be ResoureUrn instead? What's the big advantage in using classes? Finally, semi-global containers such as Contexts are convenient but do not help so much when it's time to untangle the life-cycle of objects: object A might require object B which hasn't been published on a Context yet. Debugging this requires one to go through quite a bit of code to see what gets instantiated/registered and when. And that's not counting modules code. Couldn't the Context class provide some monitoring functionality, so that object A can wait for object B to become available? I'm really asking this assuming there are good reasons. But I'm also hoping to offer the chance to review the reasons and make sure they are as good as expected.

For the Config work, there seems to be quite a bit of confusion on what architecture configs should have and what are the priority on this regard. In issue #2668 (FlexibleConfig) I proposed a <ResourceUrn, Setting> map-based architecture to address runtime storage/retrieval of settings, to be used both by engine and modules. My aim in the design was effectivness and simplicity: it has to work well but also be intuitive - we don't want developers having to waste time learning how it works, there is already plenty of challenging code in Terasology. Now, never mind for a moment the PR that resulted. Chats I just had with @Flo and @Immortius point to alternative approaches that at least in their minds might be more desirable. Of course it is somewhat frustrating to have opened an issue on the matter months ago and only now find out I might be going in the wrong direction and I might have dragged a GCI student with me. But never mind that either. Can @Flo and @Immortius provide some clearly laid out architecture for configs and settings that are:

• un/registered at runtime by engine and modules alike
• observable
• allow ranges and lists of choices
• separate from the UI but UI friendly.

Finally, and just to make sure: we don't want Terasology's code look like Perl scripts, right? Just to make sure we have the same priorities.

 

[Florian]

Why objects of Context are looked up by class and not resource: By using classes as return types you can let the compiler verify that you always get the correct type. Without passing a class as argument you would have to do a a cast and hope you get the type you expected.
Also you can automatically inject fields based on their type via

@In

annotation when the keys are classes. Also objects in the Context are often semi singletons which are of course best known by class name and not some resource url.

Why there is no official static way to get the Context: The idea behind the Context is that you can later say run server and client concurrently with different context instances. If there is just one static instance like currently then it is not possible. By having no offical static way of getting the Context the use of injection and passing by argument gets encouraged. If you really need to, you can get a instance of Context via CoreRegistry.get(Context.class), although I would discourage that.

 

[manu3d]

Thank you @Florian for the clarifications.

In regard to Contexts lookup, I understand what you mean in terms of convenience. However, is that worth the inflexibility? I mean, is the convenience of not having to cast what's returned and the convenience of using injection worth the possibility to store multiple instances of the same class? I.e. to write:

@In
WorldRenderer worldRenderer;

or to write

WorldRender worldRender = (WorldRenderer) context.get("engine:WorldRenderer")

Doesn't seem significantly different except that the second example is more flexible as it allows for multiple instances of the same class. What am I missing? Would that be inappropriate?

Regarding a static Context, I now understand why that is, thank you. IF the usecase for multiple top-level contexts is just server/client, could we then have Context.getClientContext() and Context.getServerContext() methods, returning null when one of them is not relevant?

But I agree we shouldn't be getting the context from the CoreRegistry. I mean, we want to get rid of the CoreRegistry, right? No point keeping dependencies on it.

 

[Rostyslav Zatserkovnyi]

An additional topic that's been mentioned on Slack is extracting Config+Context+Injection into a small helper library, either gestalt-based or standalone, that would be used by an external NUI framework and potentially other games/projects. The library would include:

• Config - the non-hard coded configuration system as mentioned and discussed above. Would include: org.terasology.flexible after it's finished and merge.
• Context - the well-known handler for utility objects. Would include: org.terasology.context.
• Injection - @In/@Share (the annotations used to mark systems as shareable) and InjectionHelper (handles actual injection). Would include: org.terasology.registry (besides CoreRegistry), as well as ReflectionUtil to help with some of the metaprogramming magic.
• Other - SimpleUri (a *simple* POJO-ish variant of ResourceUrn) is an additional requirement, and a few classes of a similar caliber might need to be extracted.

 

[eviltak]

@manu3d Don't worry about dragging me down, it's all in the spirit of open source

I agree with @Florian that Context objects should be looked up by a Class and not a ResourceUrn or similar type, for the same reason: By using Classes as keys you can let the compiler verify that you always get the correct type. As a side note, the Class approach also (emphasis on also) allows for injection via @In apart from retrieving the object via Context.get.

To elaborate on the pros of the Class approach, take the following code.

WorldRenderer renderer = (WorldRenderer) context.get("engine:Context")

I know this sort of code will be pretty hard to come by, but you must never underestimate what the human mind is capable of. The only indication that the types are invalid is the Exception raised on running the code. Depending on where this code is located and how well it is tested, it may not be called at all and may be given the green light by the author.

Now if Context uses Classes to retrieve objects instead:

WorldRender worldRender = context.get(Context.class)

Any near good linter will report the following code incorrect, and the code will raise a compiler error. It's much more easy to catch the error here. Plus, it's less verbose without losing readability. This is why I use a similar approach in #2757 (FlexibleConfig).

 

[Florian]

@In
WorldRenderer worldRenderer;

or to write

WorldRender worldRender = (WorldRenderer) context.get("engine:WorldRenderer")

Doesn't seem significantly different except that the second example is more flexible as it allows for multiple instances of the same class. What am I missing? Would that be inappropriate?

you don't win anything with that. If you need a way to get instances via code then you can do

@In
MyFactory myFactory;

and then

myFactory.createInstance();

It is just as flexible and more save. Also how would you know that the id is "engine:WorldRenderer"? How would you specify the parameters of the instance?

With a factory you could do it like this:

swordFactory.createWithEnchantment(Enchantment.EXTRA_DAMAGE);

. IF the usecase for multiple top-level contexts is just server/client, could we then have Context.getClientContext() and Context.getServerContext() methods, returning null when one of them is not relevant?

yes but we would then be limited to that use case and if you use Context.getClientContext then it is not possible to use the same code for the server. by using injection (or getting objects from a Context instance passed via argument) then it is possible to use the same code for both client and server even if both use different objects.

Edit: Also if you use classes as keys, then any rename of the class with automatically update all keys. If you use strings as keys that is not the case (except you do in string replacements too but that requires careful review).

 

[manu3d]

Ok, thank you for your reply @Florian, I won't insist any further on the matter: you thought about it all much more than I have.

 

[Cervator]

Okay some more thoughts after some discussion with @manu3d on Slack about the impacts of internationalization on Config and such Led to some neato discoveries, although in the end I could be giving into the temptation of overly fanciful below.

Ran across our Input Bindings/Settings in far more detail than I have before. I was aware we use annotations at the top of a Button class to define a hot key, including a description that can be translated, like so:

@RegisterBindButton(id = "autoMoveMode", description = "${engine:menu#binding-autoMove-mode}")
@DefaultBinding(id = Keyboard.KeyId.R, type = InputType.KEY)
public class AutoMoveButton extends BindButtonEvent {

Pretty straight forward. It is easy to go overboard on annotations but this makes sense to me. You define a binding with an id and a description, then in this case additionally assign a default for it. You end up with an entry in config.cfg like this (clutter cut out):

  "input": {
    "binds": {
      "engine": {
        "autoMoveMode": "key_r",

And this part of the settings UI does do the kind of dynamic setup that we'd like to see for rendering, with this particular key grouped under the "Movement" section.

Wait - what? How'd it know how to group under Movement? That was a sneaky part I didn't spot right away. This class is in the package:

org.terasology.input.binds.movement

Oooohhhh! Sneaky! Furthermore there is a package-info.java in that package with an InputCategory annotation in addition to the usual API annotation that flags the whole package as OK for the module sandboxing. Here are the interesting bits:

@InputCategory(id = "movement",
        displayName = "${engine:menu#category-movement}",
        ordering = {
                "engine:forwards",
                "engine:backwards",
                "engine:left",
                "engine:right",
                "engine:toggleSpeedPermanently",
                "engine:toggleSpeedTemporarily",
                "engine:autoMoveMode",
                "engine:crouchMode",
                "engine:jump",
                "engine:crouch"
        }) 
        package org.terasology.input.binds.movement;

Okay, so the actual package path may not matter that much after all, it just happens it is the perfect spot to define the movement category - if a bit hidden and spooky.

Also interesting is how this is a category definition with a translated key again. Which isn't obvious if you're just looking at a button that explicitly defines what category it belongs to. Like this button from the WoodAndStone module:

package org.terasology.crafting.event;
...
@RegisterBindButton(id = "craftInHand", description = "Craft in hand", category = "interaction")
@DefaultBinding(type = InputType.KEY, id = Keyboard.KEY_G)
public class CraftInHandButton extends BindButtonEvent {

That one just says it is in the "interaction" category, its package path differs and has no package-info.java. But the engine does have an interaction package + category which gets it registered so all the interaction category buttons know where to go.

Additionally that button is in a module, so it actually gets saved to:

  "input": {
    "binds": {
      "WoodAndStone": {
        "craftInHand": "key_g"

Now at this point all the magic in setting that up seems to happen in InputSettingsScreen, although I'm still not quite familiar with annotation magic to be sure. Gestalt asset or module may also get involved, seems like that's how all the annotated classes within a module environment are returned. That class is paired with the actual InputSettingsScreen.ui which has sort of a filler component where the categories get written into at runtime. @Rostyslav Zatserkovnyi can probably explain that better.

So! Where am I going with this. Well, we have the Config PR up, without much talk about very specific examples of how we'll actually configure something. Key bindings would be another example in addition to rendering, and it already kinda works dynamically.

@manu3d's example back in the issue request talks about Nodes wanting to publish config items for the Rendering config screen. Could that be done much like how the input bindings register themselves via annotation? Just instead of doing it at the class level, do it at the variable level? Example:

    @Configurable(id = "floatyShadows", displayName = "${engine:config#floaty-shadows}", category = "shadows", min = "1.0", max = "4.0")
    private Float floatyMcFloatFace;

    @Configurable(id = "shadowyDisplaySettings", displayName = "${engine:config#shadowy-display-settings}", category = "shadows", nested = true)
    private DisplayModeSetting mahDisplay;

Contrived example with silly names, but I wonder if there's a way we could make it work, with deduced defaults allowing you to cut down whatever you don't need to explicitly declare, like what the expected variable type is. Complex types could defer to the actual class for details (or use some other approach if it is a 3rd party class). Maybe it could even automatically figure out what FlexibleConfig to attach to somehow, maybe by registering the owning class.

@RegisterNode(configController = "RenderingConfig")
public class MyShadowNode

Would attempt to store the config in a FlexibleConfig associated with the RenderingConfig from a Context somehow.

Like the input mappings figuring out a particular setting should be associated with a module thanks to gestalt-module and annotation scanning maybe we can use some of our existing toys like that here. Although it could alternatively be cool to use config style annotations independently so those could go with an extracted Config+Context+Injection, without a direct dependency on Gestalt.

 

[Hybrid]

This is probably a silly question, but why can't we just use a string as a key and pass a class for type-checking?
You could just do something like this:

WorldRender worldRender = (WorldRenderer) context.get("engine:WorldRenderer",WorldRender.class)

Slightly verbose, but not really a big deal.
And then you could just check if the two classes match inside the Context.

 

[Hybrid]

Okay, so, apparently the AssetManager works exactly like this. So, my question is this: what role is a Context supposed to fulfill, that the AssetManager can't?
If we're worried about it being a global single-source-of-struth, properly namespacing the assets could deal with that.

 

[Florian]

The asset manager allows you to access all types of assets like images, sounds, prefabs (data).

Context allows you basically to get classes that provide you with functionality. E.g. you can get a AssetManager or an EntityManager from a Context.

So you should never have the need to get 2 instances of a class from a Context. If there is more of something you can simply introduce a manager class for that more of something and make that manager class available via the Context. I suggest you simply have a look at the code to see more examples. Typically you also don't get things manually from the Context instance but via @In annotation.

 

[oniatus]

I had a look at #2618 again and I think that extended module permissions/a structure between modules and the engine as proposed in #1703 would close the gap.

The main use-cases I see here are internet access (connect to IRC, store player stats in cloud, authenticate at a Terasology OAuth service, ...) and database access (e.g. for a user log like the minecraft logblock or more expensive data like player property areas which should not be kept in memory all the time but still may be needed or updated independent of chunks/sectors).

From a security perspective, i'd like to have something like a module but with extended permissions, which can be added to other modules and must be confirmed by the user.
This could be as simple as the Android permissions, where you simply allow "Network Access" without bothering the user with stuff like class- or package whitelists.

 

[SoniEx2]

I'm thinking we should get a method-based injection system. A "best-effort" injection system. It injects whenever possible and may inject multiple times.

The obvious caveat being that you shouldn't re-inject in order to change permissions, since a module could just keep the old instance for the sake of permissions (but even if you use field-based injection a module could just move the instance into a non-injectable field, so it's not really an issue).

Such injection system should also support specifying the target module of the injection.

There should also be a hook/event for when a module/injection target is loaded/unloaded, allowing injection as soon as possible, and allowing cleanup (force-closing open sockets, files, database connections, etc).

This would be used not just for cross-module sandboxing, but also for the socket API (and anything else that needs per-module permissions, for that matter), which would be implemented in the engine.

Never mind, found a better way to do it. Way more hackish, way simpler, and it just works. It's almost like the system was designed for it.