Skip to content
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

User model: Refactor the config handling #257

Open
msm-code opened this issue Dec 29, 2021 · 4 comments · May be fixed by #433
Open

User model: Refactor the config handling #257

msm-code opened this issue Dec 29, 2021 · 4 comments · May be fixed by #433
Assignees
Labels
next-sprint Scheduled for work (CERT.PL internal) zone:backend Backend oriented tasks
Milestone

Comments

@msm-code
Copy link
Contributor

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-cert
Copy link
Member

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 msm-cert added the zone:backend Backend oriented tasks label Sep 16, 2024
@msm-cert msm-cert added priority:high Priority: high next-sprint Scheduled for work (CERT.PL internal) and removed priority:high Priority: high labels Sep 17, 2024
@msm-cert msm-cert modified the milestones: v1.4.0, v1.5.0, Sprint 1 Oct 16, 2024
@michalkrzem
Copy link
Collaborator

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 #324 (comment) ?

@michalkrzem
Copy link
Collaborator

@msm-cert
Copy link
Member

msm-cert commented Oct 22, 2024

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).

@michalkrzem michalkrzem linked a pull request Oct 30, 2024 that will close this issue
4 tasks
@michalkrzem michalkrzem linked a pull request Oct 30, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-sprint Scheduled for work (CERT.PL internal) zone:backend Backend oriented tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants