Credential option for config entries

Implement a credential option to config entries which is a list of
locations the config entry can be read from.  When specified and the
option is a string field this gets rendered as a password field in the
config UI.  If the credential is not readable from control the value of
the fields is cleared when the UI is refreshed.

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)

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<Fields> = {
@@ -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<typeof Config.jsonSchema> {
+	toRemote(remote: ConfigLocation): Static<typeof Config.jsonSchema> {
 		let fields: Record<string, FieldValue> = {};
 		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<typeof Config.jsonSchema>,
 		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<Field extends keyof Fields & string>(name: Field, location = this.location) {
+	get<Field extends keyof Fields & string>(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<Field extends keyof Fields & string>(name: Field, newValue: Fields[Field], location = this.location) {
+	set<Field extends keyof Fields & string>(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}'`);

packages/web_ui/src/components/BaseConfigTree.tsx

@@ -16,13 +16,13 @@ const { Title } = Typography;
 function getInitialValues(config: lib.Config<any>, 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<string, InputComponent>, def: lib.F
 		return <Checkbox/>;
 	}
 	if (def.type === "string") {
+		if (def.credential) {
+			return <Input.Password/>;
+		}
 		return <Input/>;
 	}
 	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<any>,
@@ -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<any>).setProp(fieldName, propName, null);
 									setConfig(newConfig);

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(