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

Stop using global state to store configuration options #159

Open
kdmccormick opened this issue Oct 18, 2023 · 0 comments
Open

Stop using global state to store configuration options #159

kdmccormick opened this issue Oct 18, 2023 · 0 comments
Labels
code health Proactive technical investment via refactorings, removals, etc.

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Oct 18, 2023

Codejail has a global variable at codejail.jail_code.COMMANDS. It's a dictionary that maps command names (like "python") to safe shell commands. An example value is:

{'python': {'cmdline_start': ['/edx/app/edxapp/venvs/edxapp-sandbox/bin/python', '-E', '-B'], 'user': 'sandbox'}}

The dictionary is populated by calling codejail.jail_code.configure, which is generally done via the codejail.django_integration middleware.

This all works fine, but the fact that COMMANDS is a global variable is not idiomatic Python. More concretely, it's a pain in the neck for unit tests: the order and timing with which unit tests are run will affect whether .configure has been called or not. This could also cause problems across Python processes in production, leading to situations where codejail is configured in one process but effectively disabled in another process without anyone knowing. Not good.

This stateful system also forces us to use a middleware in order to configure codejail in Django--that's another layer where something could go wrong.

I recommend replacing COMMANDS with either:

  • a stateless function named load_commands(), which is safe to call over and over again, which does something like this:
     try:
        from django.conf import settings
     except ImportError, AttributeError
         settings = {}
      if settings.get('CODE_JAIL'):
          return _load_command_from_django_settings(settings)
      else:
          # .... load alternative/fallback configuration options, if we're not using Django
  • putting the entire Codejail interface behind a class, so clients would need to pass the settings into it, like this:
    from django.conf import settings
    codejail = CodeJail(some_setting=settings.CODEJAIL[...], etc..)
    codejail.safe_exec(...)
@kdmccormick kdmccormick added the code health Proactive technical investment via refactorings, removals, etc. label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc.
Projects
None yet
Development

No branches or pull requests

1 participant