mquery icon indicating copy to clipboard operation
mquery copied to clipboard

User model: Refactor the config handling

Open msm-code opened this issue 4 years ago • 4 comments

Part of #23

Right now we handle config very roughly. For example let's take:

auth_default_roles = db.get_mquery_config_key("auth_default_roles")
if auth_default_roles is None:
    default_roles = []
else:
    default_roles = [
        role.strip() for role in auth_default_roles.split(",")
    ]
  1. Default value is not declared anywhere, just used ad-hoc
  2. The code parses the config key at the point of usage. This obscures the intention and may lead to duplication
  3. Last but not least, the value is not validated when changed by the user. And typos here may lock users out and even make the system unusable.

I think about something like:

default_roles = db.config.auth_default_roles 

Where the auth_default_roles is a property that does the magic (parsing and validation)

msm-code avatar Dec 29 '21 18:12 msm-code

in other words:

Change all instances of db.get_mquery_config(...) to something like db.get_config that returns a well-typed Python object.

msm-cert avatar Sep 16 '24 16:09 msm-cert

Do I understand correctly, reading other threads on this topic, that the https://github.com/bwindsor/typed-config solution will not be applicable here due to incompatibility with Docker environments? Zgodnie z https://github.com/CERT-Polska/mquery/pull/324#issuecomment-1401286103 ?

michalkrzem avatar Oct 22 '24 14:10 michalkrzem

What about solution like https://github.com/CERT-Polska/Artemis/blob/main/artemis/config.py

michalkrzem avatar Oct 22 '24 14:10 michalkrzem

Hi,

short version (as per PM), something like a class MqueryConfig with fields like this:

@property
def default_roles(self) -> List[UserRole]:
    auth_default_roles = db.get_mquery_config_key("auth_default_roles")
    if auth_default_roles is None:
        return []
    else:
        return [
            role.strip() for role in auth_default_roles.split(",")
        ]

(a typed property per wrapped config entry) will probably be OK.

that the https://github.com/bwindsor/typed-config solution will not be applicable here due to incompatibility with Docker environments

In this case the problem is that typed-config is for config files whereas this is more dynamic (it's more like program settings, I guess?) I guess we could make a provider for typedconfig that would make it possible to use typedconfig for this, but I think it's an overkill

What about solution like https://github.com/CERT-Polska/Artemis/blob/main/artemis/config.py

Interesting, I never saw Annotated[] using like this. This is similar to what I had in mind (see above), but we need dynamic dispatch because settings may change (so we need properties/functions instead of static fields).

msm-cert avatar Oct 22 '24 20:10 msm-cert