Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions docs/hook-event-brief.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
# Hook and Event subsystem: scan and propose

**Scan and propose only. Do not write implementation code. Do not open a PR.**

Read `docs/refactor-facts.md` first; it outranks this file.

---

## Why this is different from the phase work

The hook system is the plugin ABI. Every bundled plugin and every third-party
plugin registers through `EventManager::register()` and is dispatched through
`HookManager::processEvent()`. ADR 0013 promises plugin authors that code
written against 1.6 keeps working for all of 1.6.

If this goes out wrong, every plugin on every server breaks at once, and the
symptom is silent: a hook that fails to register logs a line and returns
normally. Nobody finds out until a feature quietly stops happening.

So the bar here is higher than "the tests pass." The bar is: **no plugin, core
or third-party, needs editing.** Anything that would require a plugin author to
change a line is not a refactor, it is a breaking change, and it needs to be
identified as one and decided on separately.

---

## Goals, in priority order

1. **Correctness.** Several things below are wrong today, not merely ugly.
2. **The contract stays.** `register()`, `processEvent()`, `notify()`,
`hasListeners()` keep working for existing callers.
3. **Efficiency, measured not assumed.** I want this lean in memory and code,
but do not optimize on instinct. Measure what it costs now (how many hook
and event objects are constructed per request, how much of that a typical
request ever dispatches) and report it before proposing anything that trades
clarity for speed. If the current cost is already small, say so and drop
this goal.

---

## What I have observed

These are observations, not a specification of the fix. Verify each one
independently before relying on it; if any is wrong, say so.

`packages/web/lib/fog/eventmanager.class.php`,
`hookmanager.class.php`, `event.class.php`, `hook.class.php`.

**1. `load()` distinguishes the two managers with two sequential `instanceof`
checks** (`eventmanager.class.php:218-225`). `HookManager extends EventManager`,
so a HookManager satisfies both branches. The correct file extension and
directory are reached only because the second assignment overwrites the first.
Reordering the two blocks changes behaviour.

**2. Whether a class is active is decided by a regular expression run over the
file's source text**, line by line (`eventmanager.class.php:248-252`), matching
`$active\s?=\s?true;`. The class is never loaded and the property is never read
at that point. `\s?` is zero-or-one, so spacing variations do not match, and the
scan does not distinguish code from comments.

**3. `register()` branches on `self::shortName($this)`** with a case per
subclass and a `default` arm that throws
(`eventmanager.class.php:71-103`). The parent enumerates its children by name.
The throw is caught by the surrounding handler and written to the log
(`:105-122`), so a failed registration returns normally and the caller cannot
tell. The comment above the switch records that this shape already caused a
total registration failure once during the namespace migration.

**4. `HookManager` inherits `notify()`**, which iterates listeners as objects
(`$element->active`, `$element->onEvent()`) while `HookManager` stores them as
`[$object, 'method']` arrays. Determine whether anything reaches it.

**5. `notify()` treats "no listeners registered" as an exception**
(`eventmanager.class.php:175-177`) that is caught, logged, and returns false.
`processEvent()` handles the same condition with a bare `return`
(`hookmanager.class.php:89-91`). Same situation, two behaviours, one of which
writes a log line on what is probably the common case.

**6. `processEvent()` force-activates any listener whose file path contains the
substring `plugins`** (`hookmanager.class.php:103-105`), regardless of its
`active` property. Note before concluding anything: `FOGBase::fileitems()`
builds `$regex_pgrep` from `self::$pluginsinstalled`
(`fogbase.class.php:3648-3654`), so plugin files appear to be filtered to
installed plugins before reaching this point. Establish whether that makes the
force-activation redundant, or whether there is a path where it is the only
thing making a listener run.

**7. `Hook extends Event`.** Assess whether a hook is genuinely a kind of event,
or whether the two are peers with different dispatch contracts. `processEvent()`
merges arguments and calls a method that can mutate them through references
embedded in the payload; `notify()` calls `onEvent()` and discards the result.

---

## Required before you propose anything

**Inventory the contract.** Every call site of `register()`, `processEvent()`,
`notify()` and `hasListeners()` in `fogproject` AND in `fog-plugins`. What
shapes of listener are actually passed, what payloads are actually mutated. The
contract is what the callers do, not what the docblocks say.

**Characterize current behaviour before changing it.** Write tests that pin what
happens today, including the parts that are wrong. Then a behaviour change shows
up as a test that has to be edited, deliberately, rather than as a silent
difference.

**For each defect, determine whether anything currently depends on it.** This is
the part I care most about. Two concrete examples:

- If activation stops being a source-text regex and starts reading the property,
a hook whose property is `false` but whose file contains the literal string
somewhere (a comment, an example) is active today and would go inactive.
Search `fog-plugins` and core for any file where the regex result and the real
property value disagree.
- If the path-substring force-activation is removed, any plugin hook whose
`$active` is not `true` becomes inactive. Search `fog-plugins` for hooks that
do not set it true.

Report what you find. Do not assume the answer is zero.

**Say what a third-party plugin cannot see.** `fog-plugins` is the plugins I
ship. Servers run plugins I have never read. Any change whose safety argument
depends on inspecting plugin source is weaker than one that does not, and I need
that distinction stated explicitly for each proposal.

---

## What to bring back

A plan in the same form as the phase plans: `VERIFIED` / `INFERRED` / `UNKNOWN`
throughout, commit-by-commit with the tree green after every commit, a shell
command per claim, alternatives considered and rejected, and the blast radius of
each step for plugin authors.

Plus, and separately from the plan:

1. **The defect list as you found it**, including anything above that turns out
to be wrong or that you found and I did not.
2. **The decisions that are mine, not yours.** Where fixing a defect would
change behaviour something might rely on, do not pick. Present the options
and what each costs. Bugs that are load-bearing for somebody are not
automatically bugs to fix.
3. **The measurement**, per goal 3 above.
4. **Whether this warrants an ADR.** It changes an ABI that ADR 0013 made
promises about, so my instinct is yes, but argue it.

Ask me questions before planning if the code does not answer something.

**End with: which claim in this plan, if false, would hurt most?**
248 changes: 248 additions & 0 deletions docs/refactor-brief.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
# FOG working-1.6: Modernization Planning Brief

You are producing **plans, not implementations**. Read this whole file before
doing anything.

There are four phases. They are ordered by dependency, not by importance.
**Plan one phase at a time and stop for my approval at each gate.** Do not
plan Phase 2 until I have approved Phase 1. If you find yourself planning
across phase boundaries, stop and tell me why the boundary is wrong.

---

## Ground rules (apply to every phase)

Before writing anything, read the actual code. Do not plan from assumption.
Every factual claim you make about how FOG currently works must cite
`file:line`, and you must have actually opened that file.

Mark every statement as one of:

- `VERIFIED` - I read this in the code, here is file:line
- `INFERRED` - this follows from what I read but I did not confirm it
- `UNKNOWN` - I could not determine this

If the INFERRED and UNKNOWN sections are short, you have not looked hard
enough. I would rather have twenty honest unknowns than a confident plan
that is wrong in three places.

Each phase plan must contain:

1. A **commit-by-commit sequence** where the application boots and the test
suite passes after every single commit. No step may leave the tree broken
pending a later step.
2. For each commit, a **shell command** (grep, test, curl) I can run to verify
it did what it claims. If a claim cannot be reduced to a command, say so
explicitly rather than inventing one.
3. **Alternatives considered and rejected**, with reasons. Match the style of
`docs/adr/0009-plugins-become-installable-artifacts.md`.
4. Every **irreversible step** and every **data migration** called out
separately, each with its rollback story.
5. What breaks for a **third-party plugin author**, and the deprecation window.

Ask me questions before planning if the codebase does not answer something.
Do not guess and proceed.

At the end of each phase plan, answer this directly: **which claim in this
plan, if false, would hurt most?** I will verify that one by hand before
approving.

---

## Why this order

- Composer must exist before the auth work, because the entire argument for
provider-based auth is not hand-rolling JWT validation.
- The backslash-prefix pass must happen before anything else is written, or
new code needs the same treatment later.
- Site and RBAC must settle before auth, because auth maps IdP claims to
roles and those roles need to be stable first.
- Namespacing goes last because it touches every file. Anything in flight
during it becomes a merge conflict factory. The bridge autoloader from
Phase 0 is what allows it to be last without blocking the rest.

---

## PHASE 0: Foundation

Read `packages/web/commons/init.php` (the autoloader, `autoload()` around
line 256) and `packages/web/lib/fog/fogbase.class.php` `getClass()` around
line 531.

Plan three independent changes, as separate PRs, in this order:

**0.1 - Backslash-prefix every unqualified global class reference** across
`packages/web`. `\Exception`, `\DateTime`, `\DateTimeZone`, `\PharData`,
`\ReflectionClass`, `\PDO`, `\RecursiveIteratorIterator`, and anything else
you find. In the global namespace this is a no-op, which is the point: it
must be reviewable as a purely mechanical diff with zero behaviour change.
Give me the exact count per class before proposing the change.

This exists because inside a namespace, `catch (Exception $e)` silently
becomes `catch (FOG\Exception $e)`, which fails only on the error path, only
at runtime. Defusing it now, in a boring standalone commit, is what turns
the scary part of Phase 3 into a non-event.

**0.2 - Bridge autoloader.** Make the existing autoloader resolve a
namespaced request by falling back to the short name and `class_alias`ing
the result, so `FOG\User` works before any file is namespaced. Handle the
case where the included file does not declare the expected class name.
Note that `autoload()` builds a lowercased basename map, so a namespaced
lookup currently misses entirely and silently.

**0.3 - composer.json** with PSR-4 `FOG\` mapped to a new, empty
`packages/web/src/`, registered alongside the existing autoloader rather
than replacing it. `spl_autoload_register` is a chain; both can coexist.

For 0.3 the real question is operational, not code. FOG installs on
air-gapped and firewalled networks and currently vendors nothing. Lay out
committing `vendor/` to the repo versus running `composer install` at
install time: failure modes, repo size, update story, what happens when
Packagist is unreachable mid-install, and how `bin/fetch-plugins.sh`
interacts with either choice. Recommend one and argue for it.

Also answer: does anything compare class names as strings, e.g.
`get_class($x) == 'Host'` or `::class` against a literal? Those do not
survive aliasing. Find them all and list them.

**GATE. Stop here for approval.**

---

## PHASE 1: Site into core, alongside RBAC

Read `docs/adr/0006`, `docs/adr/0009`,
`packages/web/lib/fog/authorization.class.php`, and the `site` plugin at the
external plugin root.

Plan moving site's scope enforcement into core alongside RBAC.

**Design constraint I have already decided: no enable flag.** Sites are
unconditional. Zero sites defined means one implicit site containing
everything, which is today's behaviour and today's out-of-box experience.
Defining a site is what makes the boundary start to mean something. A
boundary that exists only when a boolean is true is still a boundary that
can be absent. Argue against this if you think I am wrong, but argue
*before* planning, not during.

Answer these from the code specifically:

- `objectInScope()`, `requirePageObjectScope()` and the Mass variant
currently depend on the site plugin's hook listeners. What exactly happens
to each when no listener is registered? Quote the code path.
- Do `registry()` or `purgePermissions($nodePrefix)` derive permission node
names from class names or plugin names? If yes, moving site into core
changes those strings, and that is a data migration that could silently
revoke access on live servers. **This is the thing I am most worried
about. Treat it as the headline question of this phase.**
- The site plugin owns tables with real customer data. Plan the migration
for three cases: servers with the plugin installed, servers that never had
it, and a server where the migration fails halfway through.

The plugin must keep existing on disk for at least one release. Plan what it
degrades into: a shim, a no-op, or a hard error telling the admin to remove
it.

Related, and cheap: there is no `'site'` string anywhere in
`plugin.class.php` or `pluginmanagement.page.php`, so nothing stops a future
commit adding `fog_max` to site's manifest and silently deleting the
security boundary via auto-deactivation. Add a CI test asserting site
declares no `fog_max`, regardless of what else this phase does.

**GATE. Stop here for approval.**

---

## PHASE 2: Authorization providers

Read `user.class.php` `login()`, particularly the `USER_LOGGING_IN` hook
around line 127 and the `isExternal` guard at 144-153. Read
`authorization.class.php`. Read the `ldap` plugin's hooks.

`USER_LOGGING_IN` already works for credential-presenting sources like LDAP
and AD. It cannot express OIDC, because there is no password: the browser
leaves for the IdP and returns with an authorization code. Plan the second
seam.

**Scope: OIDC only, not SAML.** OIDC covers Google, Microsoft Entra, Okta,
Authentik, Keycloak, Auth0, and GitHub is close enough to share most of the
code. SAML means a large dependency tree for a project that currently
vendors nothing. Justify or challenge that scoping.

Required:

- **Split `login()`** into `authenticate()` and `establishSession(User)`.
Right now the only way to create a session is to know a password. Show me
the exact split against the real method body, including remember-me
cookies and `UserAuth` token handling. This refactor is worth doing on its
own merits regardless of SSO.
- **A route reachable without a session** for the OIDC callback. Read the
router and `resolvePagePermission()` and tell me whether the
unregistered-page fallback is safe to rely on for an auth endpoint, or
whether it is a compatibility stance that will be tightened later and
break silently.
- **Provisioning policy: default deny.** A successful IdP login for a user
with no existing FOG account fails with a clear message. JIT provisioning
is a later switch, not the default. Claims map to RBAC roles, never to the
legacy `type` field that `USER_TYPE_HOOK` rewrites.
- **Break-glass.** Interactive SSO can never be the only way in. The FOG
client, storage nodes and API consumers do not do browser redirects, and
an IdP outage must not lock every admin out of the imaging server during
the same incident that made them need it. Show how `effectiveAdminExists()`
style reasoning applies to auth sources.
- **Library choice** for JWT and JWKS. Hand-rolling this is how `alg: none`
bugs happen. Recommend a specific package, justify it on dependency
weight, and confirm it works under whatever vendoring decision came out of
Phase 0.

This will be the first genuinely third-party-shaped plugin: it needs a
route, a login page contribution, a settings page, and a session. Where does
ADR 0009's plugin system fail to support it? That gap is more interesting to
me than the OIDC code itself.

**GATE. Stop here for approval.**

---

## PHASE 3: Namespacing

Read `commons/init.php` `autoload()`, `fogbase.class.php` `getClass()`, and
the class hierarchy under `packages/web/lib/fog`.

Phase 0 is merged by now: globals are backslash-prefixed, the bridge
autoloader resolves namespaced names via `class_alias`, and composer PSR-4
maps `FOG\` to `packages/web/src/`.

Plan migrating 227 classes with `class_alias` in **both directions**, so old
and new names work simultaneously and the app works after every commit.

- **Sequence: leaves first**, `FOGBase` / `FOGController` /
`FOGManagerController` last, since everything descends from those. Derive
the actual dependency graph from the code. Do not guess at what is a leaf.
- **`getClass()` has 491 call sites and 136 distinct names.** Convert it to
a short-name-to-FQCN map so those call sites never need editing. This is
the whole reason the migration is tractable: one chokepoint controls the
overwhelming majority of class references.
- While in there, remove the `global $sub` that steers `Storage` to
`StorageNode` or `StorageGroup`. Tell me what that breaks. It is a
landmine independent of namespacing.
- **Namespace layout:** propose one and justify it. Flat `FOG\`, or
`FOG\Model`, `FOG\Manager`, `FOG\Page`, `FOG\Hook`? I lean toward
mirroring the existing directory structure but I want your argument, not
my assumption confirmed.
- **Case sensitivity:** the current map lowercases everything, which hides
any inconsistent casing across those 491 string references. PSR-4 is
case-sensitive and will not hide it. Find the inconsistencies before they
become runtime failures.
- **Plugins:** aliases stay through all of 1.6. Tell me what a plugin author
should write today, and draft the 1.7 deprecation note.

Watch for permission nodes, API routes, or database columns derived from
class names. An alias does not change what is already stored in the
database.

---

## Reminder

Do not plan more than one phase per response. Stop at each gate.
Loading
Loading