feat: Use Environment variables for secrets capabilities #1702

[SrinivasanTarget]

Addressing #1702

Environment variable tab:

Usage: "appium:testing": "${asdf}",

We will address export and import of the secrets in a different PR.

[mykola-mokhnach]

I am not sure if it makes sense to make all env variables secret. I assume there might be some which values we still would like to inspect in plain text

[SrinivasanTarget]

I am not sure if it makes sense to make all env variables secret. I assume there might be some which values we still would like to inspect in plain text

Currently both environment variables and secrets are same and in frontend we just mask it whereas it remains plaintext in appium logs unless user explicitly filters it in logs.

[eglitise]

Also I don't really see the need of renaming reducers/Inspector.js to reducers/Inspector.jsx. The Redux website does not use this extension in their examples, and it would look out of place among its adjacent .js files.

[SrinivasanTarget]

@eglitise please help review

[eglitise]

Not sure if this code is necessary

[eglitise]

As mentioned above, I don't think this code is necessary because we only care about loading the env var values in the context of an active session. Quitting a session won't delete them from the persistent settings in any case.

[eglitise]

Also, maybe it's better to use a Map for storing the env vars, instead of an array of objects? It should simplify/speed up the duplicate key/value checks.

[SrinivasanTarget]

@eglitise Can you please help review this as we have other PR's lined up?

[eglitise]

@SrinivasanTarget I will check this once I have some time, likely this weekend. I don't see how this should block you from creating new PRs though.

[eglitise]

One thing I can point out right now though - the approach of storing these vars in persistent settings. I thought about it some more and came to the conclusion that my suggestion about this was wrong. I'm not sure why these vars should be retained after quitting the session, and we can just reapply them when starting the next session. That lead me to think that the best way to store them would be in the saved session object, alongside the server and capability details.
What do you think @SrinivasanTarget @saikrishna321 ?

[eglitise]

This specific key is not used anymore

[SrinivasanTarget]

@SrinivasanTarget I will check this once I have some time, likely this weekend. I don't see how this should block you from creating new PRs though.

No worries take your time. I wanted to avoid conflicts as i will touching the same files.

[SrinivasanTarget]

One thing I can point out right now though - the approach of storing these vars in persistent settings. I thought about it some more and came to the conclusion that my suggestion about this was wrong. I'm not sure why these vars should be retained after quitting the session, and we can just reapply them when starting the next session. That lead me to think that the best way to store them would be in the saved session object, alongside the server and capability details.
What do you think @SrinivasanTarget @saikrishna321 ?

@eglitise I would prefer to keep it persistent so that between multiple session creations users doesnt have to re-enter env variables again and save it. Also what if server (inspector as plugin) is hosted at a central place in an org and environment variables (aka secrets). do correct me if im wrong.

[eglitise]

@SrinivasanTarget the user would not need to re-enter the env vars because they would be stored in the saved session object. They would only need to select the entry in the Saved Capability Sets tab (which should be renamed, but not in this PR), and the Inspector would apply the stored env vars the same way as it currently applies server and capability details. This approach would also allow specifying different sets of env vars for different session objects.

[eglitise]

You can just pass {...props} for consistency with the other tabs. Note that the imported method names in EnvironmentVariables will need to be changed, but this is better for tracking them anyway

[eglitise]

This code can be removed since the Session reducer handles this functionality. The only use of this code is in loadEnvironmentVariables in actions/Inspector.js, but as I commented under that code, that method can also be removed.

[eglitise]

The default value here can be changed to new Map(), so you can avoid unnecessary type conversion in the set/add/delete actions. Note that the add/remove methods in actions/Session.js would also need to be adjusted to handle this.

[eglitise]

This method isn't used anywhere

[eglitise]

This should probably be renamed to setEnvironmentVariables for consistency with setVisibleProviders. I was initially confused by its call in components/Session/Session.jsx.

[eglitise]

I don't see why these are retrieved from the inspector reducer (they aren't relevant anymore at that point). The variable also has a default value, so the additional empty array is not needed.

Suggested change

	const currentEnvVars = getState().inspector.environmentVariables || [];
	const currentEnvVars = getState().session.environmentVariables;

[eglitise]

Same here

Suggested change

	const currentEnvVars = getState().inspector.environmentVariables || [];
	const currentEnvVars = getState().session.environmentVariables;

[eglitise]

Handling for default value is not needed since it is guaranteed to be set

[eglitise]

I'm not really a fan of the Card view, how about this?
I've also adjusted the table for better handling of more rows (added starting point for scroll + sticky header)

Suggested change

	import {DeleteOutlined, PlusOutlined, SettingOutlined} from '@ant-design/icons';
	import {Button, Card, Input, Popconfirm, Space, Table, Tooltip} from 'antd';
	import {useState} from 'react';
	import SessionStyles from './Session.module.css';
	const EnvironmentVariables = ({t, envVars, addVariable, deleteVariable}) => {
	const [newVar, setNewVar] = useState({key: '', value: ''});
	const tableData = Array.from(envVars, ([key, value]) => ({key, value}));
	const columns = [
	{
	title: t('Variable Name'),
	dataIndex: 'key',
	key: 'key',
	width: '40%',
	},
	{
	title: t('Value'),
	dataIndex: 'value',
	key: 'value',
	width: '40%',
	render: (text) => <Input.Password value={text} readOnly />,
	},
	{
	title: t('Actions'),
	key: 'action',
	width: '20%',
	render: (_, record) => (
	<Tooltip zIndex={3} title={t('Delete')}>
	<Popconfirm
	zIndex={4}
	title={t('confirmDeletion')}
	placement="topRight"
	okText={t('OK')}
	cancelText={t('Cancel')}
	onConfirm={() => deleteVariable(record.key)}
	>
	<Button icon={<DeleteOutlined />} />
	</Popconfirm>
	</Tooltip>
	),
	},
	];
	const handleAddVariable = () => {
	if (!newVar.key || !newVar.value) {
	return;
	}
	// Check for duplicate keys is no longer needed since Map handles this automatically
	addVariable(newVar.key, newVar.value);
	setNewVar({key: '', value: ''});
	};
	return (
	<Card
	title={
	<span>
	<SettingOutlined /> {t('Environment Variables')}
	</span>
	}
	className={SessionStyles['interaction-tab-card']}
	>
	<div className={SessionStyles.container}>
	<div className={SessionStyles.addForm}>
	<Space.Compact style={{width: '100%'}}>
	<Input
	placeholder={t('Variable Name')}
	value={newVar.key}
	onChange={(e) => setNewVar({...newVar, key: e.target.value})}
	/>
	<Input.Password
	placeholder={t('Value')}
	value={newVar.value}
	onChange={(e) => setNewVar({...newVar, value: e.target.value})}
	/>
	<Button
	type="primary"
	icon={<PlusOutlined />}
	onClick={handleAddVariable}
	disabled={!newVar.key || !newVar.value}
	>
	{t('Add')}
	</Button>
	</Space.Compact>
	</div>
	<Table
	columns={columns}
	dataSource={tableData}
	pagination={false}
	rowKey="key"
	size="small"
	/>
	</div>
	</Card>
	);
	};
	export default EnvironmentVariables;
	import {DeleteOutlined, PlusOutlined} from '@ant-design/icons';
	import {Button, Col, Form, Input, Popconfirm, Row, Table, Tooltip} from 'antd';
	import {useState} from 'react';
	const EnvironmentVariables = ({t, envVars, addVariable, deleteVariable}) => {
	const [newVar, setNewVar] = useState({key: '', value: ''});
	const tableData = Array.from(envVars, ([key, value]) => ({key, value}));
	const columns = [
	{
	title: t('Variable Name'),
	dataIndex: 'key',
	key: 'key',
	width: '45%',
	},
	{
	title: t('Value'),
	dataIndex: 'value',
	key: 'value',
	width: '45%',
	render: (text) => <Input.Password value={text} readOnly />,
	},
	{
	title: t('Actions'),
	key: 'action',
	width: '10%',
	render: (_, record) => (
	<Tooltip zIndex={3} title={t('Delete')}>
	<Popconfirm
	zIndex={4}
	title={t('confirmDeletion')}
	placement="topRight"
	okText={t('OK')}
	cancelText={t('Cancel')}
	onConfirm={() => deleteVariable(record.key)}
	>
	<Button icon={<DeleteOutlined />} />
	</Popconfirm>
	</Tooltip>
	),
	},
	];
	const handleAddVariable = () => {
	if (!newVar.key || !newVar.value) {
	return;
	}
	// Check for duplicate keys is no longer needed since Map handles this automatically
	addVariable(newVar.key, newVar.value);
	setNewVar({key: '', value: ''});
	};
	return (
	<>
	<Row gutter={8}>
	<Col span={11}>
	<Form.Item>
	<Input
	placeholder={t('Variable Name')}
	value={newVar.key}
	onChange={(e) => setNewVar({...newVar, key: e.target.value})}
	/>
	</Form.Item>
	</Col>
	<Col span={11}>
	<Form.Item>
	<Input.Password
	placeholder={t('Value')}
	value={newVar.value}
	onChange={(e) => setNewVar({...newVar, value: e.target.value})}
	/>
	</Form.Item>
	</Col>
	<Col span={2}>
	<Form.Item>
	<Button
	block
	type="primary"
	icon={<PlusOutlined />}
	onClick={handleAddVariable}
	disabled={!newVar.key || !newVar.value}
	>
	{t('Add')}
	</Button>
	</Form.Item>
	</Col>
	</Row>
	<Table
	columns={columns}
	dataSource={tableData}
	pagination={false}
	sticky={true}
	rowKey="key"
	scroll={{y: 'calc(100vh - 34em)'}}
	size="small"
	/>
	</>
	);
	};
	export default EnvironmentVariables;

[eglitise]

If you agree with my suggestion for components/Session/EnvironmentVariables.jsx, all of these changes can be removed