CTFd Plugins conflict too much

miyoyo commented 8 months ago

CTFd's plugin system bases itself on python features, notably, the fact that functions can be modified, wrapped, and replaced entirely at any time.

While this is a good thing for a total conversion, this causes issues when trying to operate multiple plugins, or to keep plugins up to date, as this means that plugins MUST depend on internal implementation details of CTFd.

Some plugins, like challenge, are even depended upon by internal code, via the CHALLENGE_CLASSES global.

I'm opening this issue as more of a tracking issue than anything, as I aim to make the CTFd plugin architecture better, with the objectives of:

Being able to run with zero plugins
Make plugins behave like they are native parts of CTFd, without having to actually inject itself in native parts
Allowing plugins to extend the listed classes (AKA these in a table) without having to override templates, hack through the DB, or other similar behavior
Ultimately, move something big, such as MLC, to the plugin architecture.

I'd gladly take suggestions for things to change, but currently, here's what I've thought of:

[x] Allow plugins to show up in the config page
[ ] Add either an extension field on User, or a method to get all attributes from one user that are stored by the plugins
[ ] Flexibilize the table pages, to allow for easier extension without having to manually support everything in the theme
[ ] Add an internal event system to allow plugins to react to actions (such as flag submitted, user logged in, etc) without having to rely on database listening
[ ] Allow plug-ins to depend on other plug-ins being loaded
[ ] Support translations for plugins

Pull requests:

2509

If you have more ideas, lmk

pve commented 8 months ago

Related, possibly, is how to deploy them consistently while tracking upstream. Here is our approach for that: https://gitlab.com/jointcyberrange.nl/ctfd-docker-with-plugins/-/blob/main/Dockerfile?ref_type=heads comments welcome.

pl4nty commented 8 months ago

I did something similar, loading plugins from GitHub https://github.com/pl4nty/containers/blob/main/ctfd%2FDockerfile

ColdHeat commented 8 months ago

I am happy to hear your ideas but try to keep in mind that the plugins are this way for a reason. Pretty much everyone has their own ideas and visions of what they want to achieve on top of CTFd and by offering a high level of flexibility, generally most people's ideas have been possible.

I am a consumer of the plugin system myself so I know it's not perfect but in many situations it has been good to me.

miyoyo commented 8 months ago

I don't disagree with the ability to hotpatch anything, it's a powerful feature that python lets us use.

The issues I see are mostly related to the ability to combine plug-ins, as two plug-ins that replace the same piece of code can have different interactions depending on load order, some may not load at all, partially load, or crash ctfd.

Hot patching also has the problem of relying on implementation details, which makes most plug-ins version dependent.

My vision with this issue is to expose some explicit hooks to plug-ins, defining a strict API for it, so that the surface area of what has to be hotpatched is minimized, and therefore the risk of conflict is minimized.

frankli0324 commented 7 months ago

I attempted implementing plugin dependencies (1. dependencies between plugins, 2. managing plugin dependencies with pip by packaging plugins into python packages) earlier but I ended up in a solution I too am not satisfied with https://github.com/CTFd/CTFd/pull/2225 just for your reference

to add to your list, plugins should also have the ability to provide their own translations

frankli0324 commented 7 months ago

I'm personally not in favor with this (https://github.com/CTFd/CTFd/pull/2509) change since the plugin mechanism allows registering admin pages and many could be achieved through extending templates

https://github.com/frankli0324/ctfd-whale/blob/master/templates/whale_base.html https://github.com/frankli0324/ctfd-whale/blob/master/__init__.py#L41

I think having a dedicated admin page for configuring plugins not only provides more flexibility and also less chance to be affected by CTFd changes.

ColdHeat commented 7 months ago

We can move the plug-in configuration section into a dedicated page if need be.

On Sat, Apr 13, 2024 at 9:30 AM Frank @.***> wrote:

I'm personally not in favor with this (#2509 https://github.com/CTFd/CTFd/pull/2509) change since the plugin mechanism allows registering admin pages and many could be achieved through extending templates

https://github.com/frankli0324/ctfd-whale/blob/master/templates/whale_base.html https://github.com/frankli0324/ctfd-whale/blob/master/__init__.py#L41

I think having a dedicated admin page for configuring plugins not only provides more flexibility and also less chance to be affected by CTFd changes.

— Reply to this email directly, view it on GitHub https://github.com/CTFd/CTFd/issues/2511#issuecomment-2053647238, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABITPMPQLVREVFACC4HWBDY5EXOLAVCNFSM6AAAAABFMNHRTOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJTGY2DOMRTHA . You are receiving this because you commented.Message ID: @.***>

miyoyo commented 7 months ago

I'm personally not in favor with this (https://github.com/CTFd/CTFd/pull/2509)...

This is exactly against the idea I'm having for plugins, they're currently aliens that modify the CTFd code via hot patching, and they either have to live outside of the rest of CTFd, or they have to modify something and hope nothing else modifies them.

Why do the settings exist for each page in a single place, but plugins would have to register their own page for it? That's just another place where inconsistency existed, and that change brings them closer to being native citizens of CTFd.

I don't see how this lowers flexibility, or even risk being affected by changes, if for some reason they change in the future, then yeah, it can just be refactored out to their own page.

I attempted implementing plugin dependencies...

I think dependencies are a good idea, however I don't believe they should exist within the context of the absolute horror that is pip, maybe as a later option, maybe as individual plugins, but CTFd shouldn't depend on it.

I drafted down quick notes for a dependency system a while ago which should be fairly quick to implement and easy to use, without having to use external file definitions or draft up an entire dependency resolution system:

Each plugin can depend on another plugin by calling required_dependency("plugin_name") (can also have optional dependencies with optional_dependency("plugin_name"))
- This function returns True if the plugin loads, False if it fails (only if optional=True, otherwise it exits)
When this function is called, the current plugin is marked as "building dependencies"
The next plugin is then loaded, which can, in turn, depend on other plugins
If this plugin is attempted to be loaded while it's building dependencies, we have a dependency loop, FATAL error, exit
Once the plugin's call to load() is finished, unmark it as "building dependencies"
If the plugin is depended upon after it's been initialized, skip initialization and return OK

to add to your list, plugins should also have the ability to provide their own translations

Good idea!

frankli0324 commented 7 months ago

I don't believe they should exist within

well, 1 and 2 are not dependent on each other in my previous impl, the pip is originally involved for unit test coverage, the idea is that we could achieve both mandatory and optional dependencies with a exception type, plugin load could decide wether a plugin is required or optional and choose whether to throw that exception

CTFd / CTFd

CTFd Plugins conflict too much #2511

2509