Coding Standards

Coding Standards

The one canonical statement of how code is written in this repository - the architecture rules, the comment/documentation rule, and the object-oriented rules a change is held to. This wiki page is the single source of truth. The repository's CONTRIBUTING.md points here; it does not restate these rules, so there is exactly one place to keep current.

These are not aspirations. Where a rule can be made executable it is enforced by a conformance fitness function (a test that runs in dotnet test on every PR) or a greppable lint rule; the rest are enforced by the adversarial Review Gate. Where the code and this page disagree, the code and its conformance tests win - and this page is corrected.

The ordering, when rules pull against each other, is security > minimal code > readability. We never trade a fail-closed guard for fewer lines or a nicer name.

1. Architecture rules

The plugin is one assembly whose behaviour is decomposed into a hexagonal module DAG. The full shape - the Api/<Module> layout, the dependency graph, and the dissolved kernel - is on the Architecture page; the rules a change must satisfy are:

• Module DAG, no cycles. New code lands in the right module folder + namespace (Jellyfin.Plugin.SSO_Auth.Api.<Module>). A module imports only the modules declared as its allowed dependencies; a new import edge is a deliberate change to the conformance test's edge list, and a cycle fails ApiModule_ImportsOnlyItsAllowedApiModules. A genuinely new concern earns a new module (folder + namespace + a DAG case), never a smear across the tree. The flat Api/ root stays empty (FlatApi_HoldsNoSourceFiles_EveryApiTypeLivesInAModule).

• Illegal states unrepresentable. Model decisions with more than two outcomes as closed sum types, not booleans or nullable flags, and let the compiler force exhaustive handling. LoginOutcome (Success / Rejected(PublicReason) / Denied, with no Error case) is the model to follow. The most security-critical guarantees are made constructional: an evidence type such as VerifiedIdentity can only be minted by the protocol validators (private constructor + named factory), so reaching account resolution with an unvalidated response is a compile error, not a review catch (VerifiedIdentity_IsConstructedOnlyByProtocolValidators).

• Fail closed by construction. This is a login path. A missing signature, time-bound, audience, subject, replayed assertion, unresolved identity, or an unmapped outcome case → reject or throw, never default-accept. No default branch that accepts. A throwing accessor turned into a guarded Try* accessor must have its new miss-branch explicitly re-decided as a rejection, with a negative test covering it - silently converting fail-closed into fail-open is the classic regression this rule exists to stop.

• No ambient time. Time enters through an injected clock; time-sensitive keyed state is keyed on UTC, not machine-local time (OidcAuthorizeState_IsKeyedOnUtc_NotMachineLocalTime). No DateTime.Now / ambient wall-clock reads on the decision path.

• Sealed, immutable, typed. Types are sealed unless a genuine is-a needs otherwise; state variants are immutable records (AuthorizeStates_AreImmutableVariants, SamlLoginOutcome_IsImmutable); values are typed rather than raw strings where a raw string invites a category error (ProviderScopedKey, ProviderMode parsed once at the edge). Mutable keyed state lives only inside store-like types (MutableKeyedState_LivesOnlyInsideStoreLikeTypes); the controller holds none (Controller_HoldsNoMutableStaticState).

• Parametrized data access. The plugin persists through Jellyfin's typed configuration model, not hand-built query strings; all config access routes through the ProviderConfigStore facade (a greppable lint rule enforces the single road to disk). The equivalent of "parametrized queries only" here is: no string-concatenated storage keys or query fragments - provider-scoped keys are typed values, not concatenations.

• Deliberate asymmetries stay deliberate. The rate limiter is fail-open by design (mass-lockout protection for a sole-admin server) and its result type is named so it can never be confused with a fail-closed gate. Log-forging sanitization stays inline at every log call and is never extracted into a helper - the CodeQL cs/log-forging sanitizer boundary does not survive a method boundary, so a helper would defeat the analysis, not just the style.

• One implementation per security primitive. A security decision that could be made in more than one place is made in exactly one, and the conformance suite pins where. Two rules carry this today. The SAML signature path reads and navigates an inbound document through a single XML stack, so the graph that is verified and the graph that is consumed cannot come apart (SamlSignaturePath_UsesOneXmlStackEndToEnd). The OpenID token validation basis is constructed in exactly one declared file, and what that one basis is allowed to accept is pinned beside it, so neither a second, laxer basis nor a symmetric algorithm on the allowlist can land green (EveryFileNamingTheValidationBasis_IsDeclared, ExactlyOneDeclaredFile_ActuallyConstructsTheBasis). Both are source scans and neither is exhaustive: a second construction inside a file the rule already declares is invisible to it, and so is a path that reaches the same decision without naming the library type at all. Each rule states its own limit in its doc comment, and the Review Gate owns what the scan cannot see.

2. Comments and documentation (#864)

• XML documentation is mandatory and complete on the whole surface - public and internal. Every type and member carries an XML doc comment describing what it is for. This is enforced by StyleCop SA1600 with documentInternalElements = true, so the internal helper surface is documented to the same bar as the public plugin contract, not just the types Jellyfin's plugin contract forces public. A conformance test pins both switches (SA1600 at warning-or-error and documentInternalElements = true) so the #864 internal-doc gate cannot be quietly switched off.

• Inline // comments are forbidden, with three explicit exceptions:

  1. load-bearing security "why" comments - the issue-referenced comments that guard a fail-closed decision; these are part of the invariant surface and travel verbatim with the code they guard when it moves, and reviewers treat them as acceptance criteria;
  2. analyzer-suppression justifications - the required rationale next to any deliberate suppression;
  3. conformance-test rationale - the "why this rule exists / why this asymmetry is intentional" notes in the fitness functions.

  Everything else is expressed in the XML doc or in the code itself. A comment that narrates what the code does is deleted along with the mechanics it narrated: the test for introducing a new named type is whether it lets an explanatory comment or a string convention be removed. Comments explain why, never what.

3. Object-oriented rules

• An abstraction must earn its place: ≥ 2 real polymorphic consumers. An interface or abstract base is introduced only when there are two or more genuine polymorphic consumers of it. A single-implementation interface, or an abstraction added "for future extensibility," is premature and is removed. Symmetry between the OIDC and SAML flows is expressed by identical named-step shape, not by a shared inheritance tower.

• Composition over inheritance - including in tests. Shared behaviour lives behind a shared abstraction realised composition- and interface-first, with shallow inheritance only where the relationship is genuinely is-a (never a deep inheritance chain). ProviderConfigBase, shared by OidConfig and SamlConfig, is the model to follow; a third IdP type must slot into that shared abstraction, not fork a third parallel path. This includes test code: there are no test base classes - a test that needs shared setup uses a composition helper (e.g. a factory that returns a configured value), so a test is self-contained and its setup is visible where it is used.

• Helper suffixes are a contract. A pure, single-responsibility leaf is named for what it does - *Validator, *Builder, *Mapper, *Resolver, *Policy, *Probe, *Extractor - and is internal and sealed or static, never part of the plugin's public surface and never an inheritance base. Keyed runtime state is a *Store / *Cache / *Gate / *Limiter. Stateful collaborators are *Service, internal sealed. Verbs carry semantics: TryRedeem is consuming and at-most-once, PeekCurrent is non-consuming; Try* returns nullable/bool and never throws for a client-caused condition. These suffix rules are enforced by the conformance tests.

How these are enforced

Three mechanisms, in order of how early they catch a violation:

1. Conformance fitness functions - the structural rules above run in dotnet test on every PR; a new structural property is locked in as a new fitness function in the same PR that establishes it. This is how the architecture stays consistent rather than drifting.
2. Greppable lint rules (the repo's opengrep invariants) - textual invariants that are not structural: CSPRNG-only randomness, config access only through the store facade, no raw outbound HttpClient, no controller error- detail leak. One rule per invariant, added per regression.
3. The adversarial Review Gate - everything not mechanically decidable (intent, OO fit, comment-necessity, fail-closed reasoning) is checked by the refute-by-default review lenses on every code PR, escalating to the full multi-lens gate on the login / crypto / config / pipeline surface.

See also

• Architecture - the module DAG, the dissolved kernel, and the login-flow code map these rules produce.
• Review Gate - the internal-only merge gate that enforces the non-mechanical rules, class by class.
• Security Model - the fail-closed invariants the rules protect.