diff --git a/CHANGELOG.md b/CHANGELOG.md index 05e6faa9..8ac866c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ Many thanks to the following for contributing to this release: - Added clean script to speed up publishing and debugging of build steps [#640](https://github.com/clusterio/clusterio/pull/640). - Added sub-tile option on plugin page headers [#642](https://github.com/clusterio/clusterio/pull/642). - Greatly improved plugin and module template generation [#641](https://github.com/clusterio/clusterio/pull/641). +- Added credential option to config entries to allow writing sensitive data that can't be read back remotely. ### Changes @@ -55,6 +56,7 @@ Many thanks to the following for contributing to this release: - Previously silent errors for controller.sendEvent now throw exceptions [#625](https://github.com/clusterio/clusterio/pull/625). - @clusterio/controller export InstanceInfo has added factorioVersion parameter. +- Argument order for `Config.canAccess` changed to require an access mode passed as the second argument. Many thanks to the following for contributing to this release: [@CCpersonguy](https://github.com/CCpersonguy) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index dae4fc8a..43903fbb 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -111,6 +111,11 @@ export interface FieldDefinition { * modified from. */ access?: ConfigLocation[]; + /** + * If present treat the config value as a credential and only allow reading + * from the locations specified. + */ + credential?: ConfigLocation[]; } export type ConfigDefs = { @@ -131,6 +136,12 @@ export type ConfigEvents = { fieldChanged: (name: string, curr: FieldValue, prev: FieldValue) => void; } +export enum ConfigAccess { + read = 0x1, + write = 0x2, + readWrite = 0x3, +} + /** * Collection of config entries * @@ -188,7 +199,7 @@ export class Config< Object.entries( this.constructor.fieldDefinitions ).filter( - ([name, def]) => this._checkAccess(name, def, location, false) + ([name, def]) => this._checkAccess(name, def, location, ConfigAccess.read, false) ).map( ([name, def]) => [name, defaultValue(def)] ) @@ -204,21 +215,28 @@ export class Config< * * @param name - Name of the field to check. * @param def - definition for for the field to check. - * @param remote - location of remote access to check for. + * @param location - location to check access for. + * @param mode - type of access to check for. * @param error - throw on failure to pass check. * @throws {InvalidAccess} * if access is not granted and error is true. * @returns true if access is granted * @private */ - _checkAccess(name: string, def: FieldDefinition, remote: ConfigLocation, error: boolean) { - for (let loc of [remote, this.location]) { - if (!(def.access ?? this.defaultAccess).includes(loc)) { - if (error) { - throw new InvalidAccess(`Field '${name}' is not accessible from ${loc}`); - } else { - return false; - } + _checkAccess(name: string, def: FieldDefinition, location: ConfigLocation, mode: ConfigAccess, error: boolean) { + // If credential array is present it acts as read permission and the access array as write permission, + // if not present access array acts as a combined read/write permission check. + if ( + def.credential && mode & ConfigAccess.read && !(def.credential).includes(location) + || ( + (!def.credential || mode & ConfigAccess.write) + && !(def.access ?? this.defaultAccess).includes(location) + ) + ) { + if (error) { + throw new InvalidAccess(`Field '${name}' is not accessible from ${location}`); + } else { + return false; } } return true; @@ -263,14 +281,17 @@ export class Config< /** * Serialize the config to a plain JavaScript object * - * @param location - Location used for access control. - * @returns JSON serializable representation of the group. + * @param remote - Location this serialised representation is for + * @returns JSON serializable representation of the config. */ - toRemote(location: ConfigLocation): Static { + toRemote(remote: ConfigLocation): Static { let fields: Record = {}; for (let [name, value] of Object.entries(this.fields)) { let def = this.constructor.fieldDefinitions[name]; - if (!this._checkAccess(name, def, location, false)) { + if ( + !this._checkAccess(name, def, this.location, ConfigAccess.write, false) + || this.location !== remote && !this._checkAccess(name, def, remote, ConfigAccess.read, false) + ) { continue; } @@ -293,12 +314,12 @@ export class Config< * @param json - * Result from a previous call to .toRemote(). * @param notify - Invoke fieldChanged events if true. - * @param location - Location used for access control. + * @param remote - Location this serialised representation was received from. */ update( json: Static, notify: boolean, - location = this.location + remote = this.location ) { const valid = Config.validate(json); if (!valid) { @@ -316,7 +337,9 @@ export class Config< continue; } - if (!this._checkAccess(name, def, location, false)) { + if ( + !this._checkAccess(name, def, remote, ConfigAccess.write, false) + ) { continue; } @@ -346,30 +369,45 @@ export class Config< * Check if field can be accessed from the given location * * @param name - Name of field to check. + * @param mode - Mode to access the field as. * @param location - Location used for access control. * @returns true if field is accessible */ - canAccess(name: string, location = this.location) { + canAccess(name: string, mode: ConfigAccess, location = this.location) { + if (typeof mode !== "number") { + throw new TypeError("mode argument is required to canAccess"); + } let def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); } - return this._checkAccess(name, def, location, false); + return this._checkAccess(name, def, location, mode, false); } /** * Get value for field * * @param name - Name of field to get. - * @param location - Location used for access control. + * @param remote - Location this value is being read from. * @returns Value stored for the field. */ - get(name: Field, location = this.location) { + get(name: Field, remote = this.location) { let def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); } - this._checkAccess(name, def, location, true); + // If remote access is requested require it to be able to read the field + if (remote !== this.location) { + this._checkAccess(name, def, remote, ConfigAccess.read, true); + } + // Require that the field is either read or writable locally so that you can get the value + // back of write only fields recently written to. + if ( + !this._checkAccess(name, def, this.location, ConfigAccess.write, false) + && !this._checkAccess(name, def, this.location, ConfigAccess.read, false) + ) { + throw new InvalidAccess(`Field '${name}' is not accessible from ${this.location}`); + } return this.fields[name]; } @@ -377,22 +415,25 @@ export class Config< /** * Set value of field * - * The field must be defined for the config group and the value is + * The field must be defined for the config and the value is * type checked against the field definition. * * @param name - Name of field to set. * @param newValue - Value to set for field. - * @param location - Location used for access control. + * @param remote - Location this field is being set from. * @throws {InvalidField} if field is not defined. * @throws {InvalidValue} if value is not allowed for the field. */ - set(name: Field, newValue: Fields[Field], location = this.location) { + set(name: Field, newValue: Fields[Field], remote = this.location) { let value: FieldValue = newValue; let def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); } - this._checkAccess(name, def, location, true); + this._checkAccess(name, def, remote, ConfigAccess.write, true); + if (remote !== this.location) { + this._checkAccess(name, def, this.location, ConfigAccess.write, true); + } // Empty strings are treates as null if (value === "") { @@ -447,12 +488,12 @@ export class Config< * Set property of object field * * Update value of stored object field by setting the specified property - * on it. The field must be defined as an object for the config group. + * on it. The field must be defined as an object for the config. * * @param name - Name of field to set property on. * @param prop - Name of property to set on field. * @param value - the value to set the property to. - * @param location - Location used for access control. + * @param remote - Location this property is being set from * @throws {InvalidField} if field is not defined. * @throws {InvalidValue} if field is not an object. */ @@ -460,13 +501,16 @@ export class Config< name: Field, prop: string, value?: unknown, - location = this.location, + remote = this.location, ) { let def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); } - this._checkAccess(name, def, location, true); + this._checkAccess(name, def, remote, ConfigAccess.write, true); + if (remote !== this.location) { + this._checkAccess(name, def, this.location, ConfigAccess.write, true); + } if (def.type !== "object") { throw new InvalidValue(`Cannot set property on non-object field '${name}'`); diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index f2a0e922..1c994170 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -16,13 +16,13 @@ const { Title } = Typography; function getInitialValues(config: lib.Config, props: BaseConfigTreeProps) { let initialValues: any = {}; for (let [name, def] of Object.entries(config.constructor.fieldDefinitions)) { - if (!config.canAccess(name)) { + if (!config.canAccess(name, lib.ConfigAccess.read)) { continue; } let value = config.get(name) as any; if (def.type === "object") { - for (let prop of Object.keys(value)) { + for (let prop of Object.keys(value ?? {})) { initialValues[`${name}.${prop}`] = JSON.stringify(value[prop]); } } else { @@ -43,6 +43,9 @@ function renderInput(inputComponents: Record, def: lib.F return ; } if (def.type === "string") { + if (def.credential) { + return ; + } return ; } if (def.type === "number") { @@ -110,7 +113,8 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { if (key.endsWith(":add.name") || key.endsWith(":add.value")) { continue; } - if (initialValues[key] === value) { + const def = config?.constructor.fieldDefinitions[key]; + if (!def?.credential && initialValues[key] === value) { changed = newChangedFields.delete(key) || changed; } else { changed = !newChangedFields.has(key) || changed; @@ -213,6 +217,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { ; } +// eslint-disable-next-line complexity function computeTreeData( control: Control, form: FormInstance, @@ -259,7 +264,9 @@ function computeTreeData( }; for (let [field, def] of Object.entries(groupDefs)) { - if (!config.canAccess(`${groupName}.${field}`)) { + const canWrite = config.canAccess(`${groupName}.${field}`, lib.ConfigAccess.write); + const canRead = config.canAccess(`${groupName}.${field}`, lib.ConfigAccess.read); + if (!canWrite && !canRead) { continue; } @@ -279,7 +286,7 @@ function computeTreeData( } tooltip={def.description} />; - for (let prop of Object.keys(value)) { + for (let prop of Object.keys(value ?? {})) { let propPath = `${groupName}.${field}.${prop}`; let restart = Boolean( def.restartRequiredProps && Number(def.restartRequired) ^ Number(restartRequiredProps.has(prop)) @@ -333,7 +340,7 @@ function computeTreeData( onClick={() => { let propName = form.getFieldValue(`${newPropPath}.name`); let propValue = form.getFieldValue(`${newPropPath}.value`); - if (!Object.prototype.hasOwnProperty.call(value, propName)) { + if (!value || !Object.prototype.hasOwnProperty.call(value, propName)) { let newConfig = ConfigClass.fromJSON(config!.toJSON(), "control"); (newConfig as lib.Config).setProp(fieldName, propName, null); setConfig(newConfig); diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 03e69c4d..efa7c6aa 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -1,7 +1,7 @@ "use strict"; const lib = require("@clusterio/lib"); const assert = require("assert").strict; -const events = require("events"); +const CA = lib.ConfigAccess; describe("lib/config/classes", function() { @@ -26,6 +26,7 @@ describe("lib/config/classes", function() { "test.bool": { type: "boolean", initialValue: false, optional: true }, "test.json": { type: "object", initialValue: {}, optional: true }, "test.priv": { access: ["local"], type: "string", optional: true }, + "test.cred": { credential: ["local"] }, }; constructor(location, fields) { @@ -72,6 +73,7 @@ describe("lib/config/classes", function() { "test.bool": false, "test.json": {}, "test.priv": null, + "test.cred": null, }); }); }); @@ -148,6 +150,7 @@ describe("lib/config/classes", function() { "test.bool": false, "test.json": {}, "test.priv": null, + "test.cred": null, "test.alpha": null, "test.beta": "decay", "test.gamma": 99, @@ -181,6 +184,7 @@ describe("lib/config/classes", function() { "test.func": 42, "test.json": {}, "test.priv": null, + "test.cred": null, "test.test": null, }); }); @@ -227,20 +231,61 @@ describe("lib/config/classes", function() { describe(".canAccess()", function() { it("should throw if field does not exist", function() { let testInstance = new TestConfig("local"); - assert.throws(() => testInstance.canAccess("invalid"), new Error("No field named 'invalid'")); + assert.throws(() => testInstance.canAccess("invalid", CA.read), new Error("No field named 'invalid'")); }); - it("should return true for fields that are accessible", function() { + it("should throw if mode is not passed ", function() { let testInstance = new TestConfig("local"); - assert.equal(testInstance.canAccess("alpha.foo"), true); + assert.throws( + () => { testInstance.canAccess("alpha.foo"); }, + new TypeError("mode argument is required to canAccess") + ); + }); + + it("should return true checking read for fields that are readable", function() { + let testInstance = new TestConfig("local"); + assert.equal(testInstance.canAccess("alpha.foo", CA.read), true); for (let field of ["test.enum", "test.test", "test.func", "test.bool", "test.json"]) { - assert.equal(testInstance.canAccess(field), true); - assert.equal(testInstance.canAccess(field, "local"), true); - assert.equal(testInstance.canAccess(field, "remote"), true); + assert.equal(testInstance.canAccess(field, CA.read), true); + assert.equal(testInstance.canAccess(field, CA.read, "local"), true); + assert.equal(testInstance.canAccess(field, CA.read, "remote"), true); } - assert.equal(testInstance.canAccess("test.priv"), true); - assert.equal(testInstance.canAccess("test.priv", "local"), true); - assert.equal(testInstance.canAccess("test.priv", "remote"), false); + assert.equal(testInstance.canAccess("test.priv", CA.read), true); + assert.equal(testInstance.canAccess("test.priv", CA.read, "local"), true); + assert.equal(testInstance.canAccess("test.priv", CA.read, "remote"), false); + assert.equal(testInstance.canAccess("test.cred", CA.read), true); + assert.equal(testInstance.canAccess("test.cred", CA.read, "local"), true); + assert.equal(testInstance.canAccess("test.cred", CA.read, "remote"), false); + }); + it("should return true checking write for fields that are writeable", function() { + let testInstance = new TestConfig("local"); + assert.equal(testInstance.canAccess("alpha.foo", CA.write), true); + for (let field of ["test.enum", "test.test", "test.func", "test.bool", "test.json"]) { + assert.equal(testInstance.canAccess(field, CA.write), true); + assert.equal(testInstance.canAccess(field, CA.write, "local"), true); + assert.equal(testInstance.canAccess(field, CA.write, "remote"), true); + } + assert.equal(testInstance.canAccess("test.priv", CA.write), true); + assert.equal(testInstance.canAccess("test.priv", CA.write, "local"), true); + assert.equal(testInstance.canAccess("test.priv", CA.write, "remote"), false); + assert.equal(testInstance.canAccess("test.cred", CA.write), true); + assert.equal(testInstance.canAccess("test.cred", CA.write, "local"), true); + assert.equal(testInstance.canAccess("test.cred", CA.write, "remote"), true); + }); + it("should return true for checking readWrite for fields that are readable and writeable", function() { + let testInstance = new TestConfig("local"); + assert.equal(testInstance.canAccess("alpha.foo", CA.readWrite), true); + for (let field of ["test.enum", "test.test", "test.func", "test.bool", "test.json"]) { + assert.equal(testInstance.canAccess(field, CA.readWrite), true); + assert.equal(testInstance.canAccess(field, CA.readWrite, "local"), true); + assert.equal(testInstance.canAccess(field, CA.readWrite, "remote"), true); + } + assert.equal(testInstance.canAccess("test.priv", CA.readWrite), true); + assert.equal(testInstance.canAccess("test.priv", CA.readWrite, "local"), true); + assert.equal(testInstance.canAccess("test.priv", CA.readWrite, "remote"), false); + assert.equal(testInstance.canAccess("test.cred", CA.readWrite), true); + assert.equal(testInstance.canAccess("test.cred", CA.readWrite, "local"), true); + assert.equal(testInstance.canAccess("test.cred", CA.readWrite, "remote"), false); }); }); @@ -259,13 +304,20 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("test.priv"), "blah"); assert.equal(testInstance.get("test.func"), 27); }); - it("should throw if field is inaccessible", function() { + it("should throw if field is remotely inaccessible", function() { let testInstance = new TestConfig("local"); assert.throws( () => testInstance.get("test.priv", "remote"), new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") ); }); + it("should throw if field is locally inaccessible", function() { + let testInstance = new TestConfig("remote"); + assert.throws( + () => testInstance.get("test.priv"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); }); describe(".set()", function() { @@ -290,12 +342,19 @@ describe("lib/config/classes", function() { ); }); - it("should throw if field is inaccesible", function() { + it("should throw if field is remotely inaccessible", function() { assert.throws( () => testInstance.set("test.priv", "bad", "remote"), new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") ); }); + it("should throw if field is locally inaccessible", function() { + testInstance = new TestConfig("remote"); + assert.throws( + () => testInstance.set("test.priv", "bad", "local"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); it("should throw if field is not in enum", function() { assert.throws( @@ -398,12 +457,19 @@ describe("lib/config/classes", function() { ); }); - it("should throw if field is inaccesible", function() { + it("should throw if field is remotely inaccessible", function() { assert.throws( () => testInstance.setProp("test.priv", "prop", "bad", "remote"), new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") ); }); + it("should throw if field is locally inaccessible", function() { + testInstance = new TestConfig("remote"); + assert.throws( + () => testInstance.setProp("test.priv", "prop", "bad", "local"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); it("should throw if field is not an object", function() { assert.throws(