Config: Improve validation and defaults logic (#16)

Config: Add improved validation + defaults + DiscoveryConfig

This commit refactors how the config types work together, and
how they implement validation. The key insight is that in OPA
the `validateAndInjectDefaults` functions are doing one or more
of four distinct tasks:

 - Default values: Handled in Swift `init` / `decode` methods.
 - Struct-local validation: Handled with a `validate() throws`
   method.
 - Validation needing external state from other parts of the
   config: Handled with custom `validateWithContext(...) throws`
   methods.
 - Default values needing external state from other parts of
   the config: Handled with custom `resolved(...) throws`
   methods.

Now that these new approaches are integrated everywhere, the
config types are harder to use incorrectly, and we do as much
validation as possible, as early in construction as possible.

Signed-off-by: Philip Conrad <philip_conrad@apple.com>

Author: philipaconrad

Package.swift

@@ -55,6 +55,10 @@ let package = Package(
             name: "SwiftOPASDKTests",
             dependencies: ["SwiftOPASDK"]
         ),
+        .testTarget(
+            name: "ConfigTests",
+            dependencies: ["Config"]
+        ),
         .testTarget(
             name: "RegoExtensionTests",
             dependencies: ["SwiftOPASDK"]

Sources/Config/BundleConfig.swift

@@ -1,4 +1,3 @@
-import AST
 import Foundation
 import Rego
 
@@ -25,6 +24,41 @@ extension OPA {
             self.exclude = exclude
         }
 
+        /// Returns a new config with the provided public keys injected,
+        /// validating that `keyID` (if set) references an existing key.
+        ///
+        /// This follows the "resolve" pattern: because `publicKeys` are defined
+        /// in a separate top-level config section, they aren't available at
+        /// decode time. The parent config calls this method during its
+        /// resolution phase to produce a fully-populated instance.
+        ///
+        /// Ported from Go's `VerificationConfig.ValidateAndInjectDefaults`.
+        public func resolved(withKeys keys: [String: KeyConfig]) throws -> BundleVerificationConfig {
+            if !keyID.isEmpty && keys[keyID] == nil {
+                throw ConfigError(
+                    code: .internalError,
+                    message: "Key ID '\(keyID)' not found"
+                )
+            }
+            return BundleVerificationConfig(
+                publicKeys: keys,
+                keyID: keyID,
+                scope: scope,
+                exclude: exclude
+            )
+        }
+
+        /// Validates constraints that require context from the parent `Config` struct.
+        /// Ported from Go's `VerificationConfig.ValidateAndInjectDefaults`.
+        public func validateWithContext(keys: [String: KeyConfig]) throws {
+            guard keyID.isEmpty || keys[keyID] != nil else {
+                throw ConfigError(
+                    code: .internalError,
+                    message: "Key ID '\(keyID)' not found"
+                )
+            }
+        }
+
         private enum CodingKeys: String, CodingKey {
             case publicKeys
             case keyID = "keyid"
@@ -47,46 +81,38 @@ extension OPA {
 
         // service is set to an empty string only for file:// resource URLs.
         public init(
-            downloaderConfig: DownloaderConfig = DownloaderConfig(),
-            service: String,
+            downloaderConfig: DownloaderConfig? = nil,
+            service: String = "",
             resource: String? = nil,
             signing: BundleVerificationConfig? = nil,
             persist: Bool? = nil,
             sizeLimitBytes: Int64 = 10 * 1024 * 1024  // 10 MB
         ) throws {
-            self.downloaderConfig = downloaderConfig
+            self.downloaderConfig = try downloaderConfig ?? DownloaderConfig()
             self.service = service
             self.resource = resource
-            // TODO: Validate file:// URLs on empty service name here too? Might be worth having a "validate()" method?
             self.signing = signing
             self.persist = persist
             self.sizeLimitBytes = sizeLimitBytes
+            try self.validate()
         }
 
         public init(from decoder: Decoder) throws {
             let container = try decoder.container(keyedBy: CodingKeys.self)
 
             // Decode DownloaderConfig properties inline
-            let trigger = try container.decodeIfPresent(PluginTriggerMode.self, forKey: .trigger)
+            let trigger = try container.decodeIfPresent(TriggerMode.self, forKey: .trigger)
             let polling = try container.decodeIfPresent(PollingConfig.self, forKey: .polling)
-            self.downloaderConfig = DownloaderConfig(trigger: trigger, polling: polling)
+            self.downloaderConfig = try DownloaderConfig(trigger: trigger, polling: polling)
 
             self.service = try container.decodeIfPresent(String.self, forKey: .service) ?? ""
             self.resource = try container.decodeIfPresent(String.self, forKey: .resource)
-            // Service is only allowed to be unset when resource is a file:// URL.
-            let isFileURL = self.resource.flatMap(URL.init(string:))?.scheme == "file"
-            guard !self.service.isEmpty || isFileURL else {
-                throw DecodingError.dataCorrupted(
-                    DecodingError.Context(
-                        codingPath: container.codingPath,
-                        debugDescription: "No service config or file:// URL was provided for bundle config."
-                    )
-                )
-            }
             self.signing = try container.decodeIfPresent(BundleVerificationConfig.self, forKey: .signing)
             self.persist = try container.decodeIfPresent(Bool.self, forKey: .persist)
             // 10 MB default
             self.sizeLimitBytes = try container.decodeIfPresent(Int64.self, forKey: .sizeLimitBytes) ?? 10 * 1024 * 1024
+
+            try self.validate()
         }
 
         public func encode(to encoder: Encoder) throws {
@@ -103,6 +129,61 @@ extension OPA {
             try container.encodeIfPresent(sizeLimitBytes, forKey: .sizeLimitBytes)
         }
 
+        /// Validates struct-local constraints.
+        public func validate() throws {
+            let isFileURL = self.resource.flatMap(URL.init(string:))?.scheme == "file"
+            guard !self.service.isEmpty || isFileURL else {
+                throw ConfigError(
+                    code: .internalError, message: "No service config or file:// URL was provided for bundle config.")
+            }
+        }
+
+        /// Validates constraints that require context from the parent `Config` struct.
+        public func validateWithContext(name: String, services: [String: ServiceConfig], keys: [String: KeyConfig])
+            throws
+        {
+            // Prevent bundle referencing a non-existent service.
+            guard self.service.isEmpty || services[self.service] != nil else {
+                throw ConfigError(
+                    code: .internalError,
+                    message:
+                        "Bundle config for '\(name)' references non-existent service: '\(self.service)'"
+                )
+            }
+            // If no service specified, require a file:// URL to load from disk.
+            guard !self.service.isEmpty || (URL(string: self.resource ?? "")?.scheme == "file") else {
+                throw ConfigError(
+                    code: .internalError,
+                    message:
+                        "Bundle config for '\(name)' has no service config or file:// URL resource config. \(self.resource ?? "(nil)")"
+                )
+            }
+
+            try self.signing?.validateWithContext(keys: keys)
+        }
+
+        /// Returns a new config with all context-dependent properties resolved.
+        ///
+        /// This follows the "resolve" pattern: some properties are defined
+        /// in separate top-level config sections and aren't available at decode
+        /// time. The parent config calls this method during its resolution phase
+        /// to produce a fully-populated instance.
+        public func resolved(withKeys keys: [String: KeyConfig]) throws -> BundleSourceConfig {
+            var resolvedSigning = try self.signing?.resolved(withKeys: keys)
+            if !keys.isEmpty && resolvedSigning == nil {
+                resolvedSigning = BundleVerificationConfig(publicKeys: keys, keyID: "", scope: "", exclude: [])
+            }
+
+            return try BundleSourceConfig(
+                downloaderConfig: downloaderConfig,
+                service: service,
+                resource: resource,
+                signing: resolvedSigning,
+                persist: persist,
+                sizeLimitBytes: sizeLimitBytes
+            )
+        }
+
         private enum CodingKeys: String, CodingKey {
             case trigger
             case polling

Sources/Config/Config.swift

@@ -13,7 +13,7 @@ extension OPA {
         // public let decisionLogs: DecisionLogsConfig?
         // public let status: StatusConfig?
         // public let plugins: [String: PluginConfig]
-        // public let keys: KeysConfig?
+        public let keys: [String: KeyConfig]
         // public let defaultDecision: String?
         // public let defaultAuthorizationDecision: String?
         // public let caching: CachingConfig?
@@ -28,11 +28,11 @@ extension OPA {
             services: [String: ServiceConfig] = [:],
             labels: [String: String] = [:],
             discovery: DiscoveryConfig? = nil,
-            bundles: [String: BundleSourceConfig] = [:]
-                // decisionLogs: DecisionLogsConfig? = nil,
-                // status: StatusConfig? = nil,
-                // plugins: [String: PluginConfig] = [:],
-                // keys: KeysConfig? = nil,
+            bundles: [String: BundleSourceConfig] = [:],
+            // decisionLogs: DecisionLogsConfig? = nil,
+            // status: StatusConfig? = nil,
+            // plugins: [String: PluginConfig] = [:],
+            keys: [String: KeyConfig] = [:]
                 // defaultDecision: String? = nil,
                 // defaultAuthorizationDecision: String? = nil,
                 // caching: CachingConfig? = nil,
@@ -42,15 +42,15 @@ extension OPA {
                 // server: ServerConfig? = nil,
                 // storage: StorageConfig? = nil,
                 // extra: [String: AnyCodable]? = nil
-        ) {
+        ) throws {
             self.services = services
             self.labels = labels
-            self.discovery = discovery
-            self.bundles = bundles
+            let discovery = discovery
+            let bundles = bundles
             // self.decisionLogs = decisionLogs
             // self.status = status
             // self.plugins = plugins
-            // self.keys = keys
+            let keys = keys
             // self.defaultDecision = defaultDecision
             // self.defaultAuthorizationDecision = defaultAuthorizationDecision
             // self.caching = caching
@@ -60,21 +60,45 @@ extension OPA {
             // self.server = server
             // self.storage = storage
             // self.extra = extra
+
+            // Some nested structures require cross-config context, so we resolve those parts out here.
+            self.discovery = try discovery?.resolved(withKeys: keys)
+            self.bundles = try bundles.mapValues({ try $0.resolved(withKeys: keys) })
+            self.keys = keys
+
+            try self.validate()
         }
 
         // MARK: - Decoder
 
         public init(from decoder: Decoder) throws {
             let container = try decoder.container(keyedBy: CodingKeys.self)
 
-            self.services = try container.decodeIfPresent([String: ServiceConfig].self, forKey: .services) ?? [:]
+            // Services can be provided as a dictionary or an array with the name fields set.
+            do {
+                let services = try container.decodeIfPresent([String: ServiceConfig].self, forKey: .services) ?? [:]
+                self.services = services
+            } catch {
+                let servicesArray = try container.decodeIfPresent([ServiceConfig].self, forKey: .services) ?? []
+                var services = [String: ServiceConfig]()
+                for (idx, service) in servicesArray.enumerated() {
+                    guard let name = service.name else {
+                        throw OPA.ConfigError(
+                            code: .internalError,
+                            message: "Missing \"name\" key for service at index \(idx) in services array")
+                    }
+                    services[name] = service
+                }
+                self.services = services
+            }
+
             self.labels = try container.decodeIfPresent([String: String].self, forKey: .labels) ?? [:]
-            self.discovery = try container.decodeIfPresent(DiscoveryConfig.self, forKey: .discovery)
-            self.bundles = try container.decodeIfPresent([String: BundleSourceConfig].self, forKey: .bundles) ?? [:]
+            let discovery = try container.decodeIfPresent(DiscoveryConfig.self, forKey: .discovery)
+            let bundles = try container.decodeIfPresent([String: BundleSourceConfig].self, forKey: .bundles) ?? [:]
             // self.decisionLogs = try container.decodeIfPresent(DecisionLogsConfig.self, forKey: .decisionLogs)
             // self.status = try container.decodeIfPresent(StatusConfig.self, forKey: .status)
             // self.plugins = try container.decodeIfPresent([String: PluginConfig].self, forKey: .plugins) ?? [:]
-            // self.keys = try container.decodeIfPresent(KeysConfig.self, forKey: .keys)
+            let keys = try container.decodeIfPresent([String: KeyConfig].self, forKey: .keys) ?? [:]
             // self.defaultDecision = try container.decodeIfPresent(String.self, forKey: .defaultDecision)
             // self.defaultAuthorizationDecision = try container.decodeIfPresent(String.self, forKey: .defaultAuthorizationDecision)
             // self.caching = try container.decodeIfPresent(CachingConfig.self, forKey: .caching)
@@ -85,33 +109,12 @@ extension OPA {
             // self.storage = try container.decodeIfPresent(StorageConfig.self, forKey: .storage)
             // self.extra = nil  // Extra is not decoded from JSON (matches Go's `json:"-"`)
 
-            // Validation for decoded config.
-            for (name, bundle) in self.bundles {
-                // Prevent bundle referencing a non-existent service.
-                guard bundle.service.isEmpty || self.services[bundle.service] != nil else {
-                    throw DecodingError.dataCorrupted(
-                        DecodingError.Context(
-                            codingPath: container.codingPath + [
-                                CodingKeys.bundles, DynamicCodingKey(stringValue: name),
-                            ],
-                            debugDescription:
-                                "Bundle config for '\(name)' references non-existent service: '\(bundle.service)'"
-                        )
-                    )
-                }
-                // If no service specified, require a file:// URL to load from disk.
-                guard !bundle.service.isEmpty || (URL(string: bundle.resource ?? "")?.scheme == "file") else {
-                    throw DecodingError.dataCorrupted(
-                        DecodingError.Context(
-                            codingPath: container.codingPath + [
-                                CodingKeys.bundles, DynamicCodingKey(stringValue: name),
-                            ],
-                            debugDescription:
-                                "Bundle config for '\(name)' has no service config or file:// URL resource config. \(bundle.resource ?? "(nil)")"
-                        )
-                    )
-                }
-            }
+            // Some nested structures require cross-config context, so we resolve those parts out here.
+            self.discovery = try discovery?.resolved(withKeys: keys)
+            self.bundles = try bundles.mapValues({ try $0.resolved(withKeys: keys) })
+            self.keys = keys
+
+            try self.validate()
         }
 
         // MARK: - Encoder
@@ -126,7 +129,7 @@ extension OPA {
             // try container.encodeIfPresent(decisionLogs, forKey: .decisionLogs)
             // try container.encodeIfPresent(status, forKey: .status)
             // try container.encodeIfPresent(plugins.isEmpty ? nil : plugins, forKey: .plugins)
-            // try container.encodeIfPresent(keys, forKey: .keys)
+            try container.encodeIfPresent(keys, forKey: .keys)
             // try container.encodeIfPresent(defaultDecision, forKey: .defaultDecision)
             // try container.encodeIfPresent(defaultAuthorizationDecision, forKey: .defaultAuthorizationDecision)
             // try container.encodeIfPresent(caching, forKey: .caching)
@@ -138,6 +141,16 @@ extension OPA {
             // Extra is not encoded to JSON (matches Go's `json:"-"`)
         }
 
+        public func validate() throws {
+            for (name, bundleConfig) in self.bundles {
+                try bundleConfig.validateWithContext(name: name, services: self.services, keys: self.keys)
+            }
+
+            for (id, keyConfig) in self.keys {
+                try keyConfig.validateWithContext(id: id)
+            }
+        }
+
         private enum CodingKeys: String, CodingKey {
             case services
             case labels