Skip to content

Add Synology DSM Security API #35305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Quentame wants to merge 4 commits into from
Closed

Add Synology DSM Security API #35305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4612218
Add Synology DSM Security API
Quentame May 5, 2020
9160f52
Remove device registry cleaning because it's automatic
Quentame May 7, 2020
ac56962
Remove useless options_updated
Quentame May 7, 2020
4799e4b
Fix manual update
Quentame May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions homeassistant/components/synology_dsm/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from datetime import timedelta

from synology_dsm import SynologyDSM
from synology_dsm.api.core.security import SynoCoreSecurity
from synology_dsm.api.core.utilization import SynoCoreUtilization
from synology_dsm.api.dsm.information import SynoDSMInformation
from synology_dsm.api.storage.storage import SynoStorage
Expand All @@ -24,8 +25,10 @@
from homeassistant.helpers.typing import HomeAssistantType

from .const import (
CONF_SECURITY,
CONF_VOLUMES,
DEFAULT_SCAN_INTERVAL,
DEFAULT_SECURITY,
DEFAULT_SSL,
DOMAIN,
SYNO_API,
Expand Down Expand Up @@ -126,6 +129,7 @@ def __init__(self, hass: HomeAssistantType, entry: ConfigEntry):
self.dsm: SynologyDSM = None
self.information: SynoDSMInformation = None
self.utilisation: SynoCoreUtilization = None
self.security: SynoCoreSecurity = None
self.storage: SynoStorage = None

self._unsub_dispatcher = None
Expand All @@ -147,11 +151,11 @@ async def async_setup(self):
)

await self._hass.async_add_executor_job(self._fetch_device_configuration)
await self.update()
await self.async_update()

self._unsub_dispatcher = async_track_time_interval(
self._hass,
self.update,
self.async_update,
timedelta(
minutes=self._entry.options.get(
CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL
Expand All @@ -163,13 +167,15 @@ def _fetch_device_configuration(self):
"""Fetch initial device config."""
self.information = self.dsm.information
self.utilisation = self.dsm.utilisation
if self._entry.options.get(CONF_SECURITY, DEFAULT_SECURITY):
self.security = self.dsm.security
self.storage = self.dsm.storage

async def async_unload(self):
"""Stop interacting with the NAS and prepare for removal from hass."""
self._unsub_dispatcher()

async def update(self, now=None):
async def async_update(self, now=None):
"""Update function for updating API information."""
await self._hass.async_add_executor_job(self.dsm.update)
async_dispatcher_send(self._hass, self.signal_sensor_update)
10 changes: 9 additions & 1 deletion homeassistant/components/synology_dsm/config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@
import homeassistant.helpers.config_validation as cv

from .const import (
CONF_SECURITY,
CONF_VOLUMES,
DEFAULT_PORT,
DEFAULT_PORT_SSL,
DEFAULT_SCAN_INTERVAL,
DEFAULT_SECURITY,
DEFAULT_SSL,
)
from .const import DOMAIN # pylint: disable=unused-import
Expand Down Expand Up @@ -250,7 +252,13 @@ async def async_step_init(self, user_input=None):
default=self.config_entry.options.get(
CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL
),
): cv.positive_int
): cv.positive_int,
vol.Optional(
CONF_SECURITY,
default=self.config_entry.options.get(
CONF_SECURITY, DEFAULT_SECURITY
),
): bool,
Comment on lines +256 to +261
Copy link
Copy Markdown
Member

@balloob balloob May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Nick. This should not be an option but just disabled by default.

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are ok to make a request to fetch security data even if we don't use it ?

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Searching to not make that with entity_registry_enabled_default

Copy link
Copy Markdown
Member

@bdraco bdraco May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the update function is smart enough to know that if it has no subscribers listening for the data that is should skip requesting those data sets.

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see

https://github.com/ProtoThis/python-synology/blob/674dfb3c943e464680c33c8bc4975139d8c72f5c/synology_dsm/synology_dsm.py#L258-L314

To think what I can do

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdraco Yes, but in the case we have one DataUpdateCoordinator per API:

When requesting a storage fetch (and with a security coordinator somewhere) the lib update will fetch storage + security

After, the security DataUpdateCoordinator will also update with fetch storage + security.

So it doubles requests !

OR,

Every DataUpdateCoordinator should have its own instance of SynoApi or SynologyDSM, it will do even more requests and will login to the NAS for every DataUpdateCoordinator, I beat it will have an error at login (logged multiple times)

Copy link
Copy Markdown
Member

@bdraco bdraco May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to some refactoring remove the the update and only have the coordinator instances for each endpoint share the single SynologyDSM so you aren't logging in multiple times.
There is a good example on how to accomplish this in nws/__init__.py:async_setup_entry

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I turn off the security sensor self._security will have already have been set here https://github.com/ProtoThis/python-synology/blob/674dfb3c943e464680c33c8bc4975139d8c72f5c/synology_dsm/synology_dsm.py#L297 so it going to continue to poll here https://github.com/ProtoThis/python-synology/blob/674dfb3c943e464680c33c8bc4975139d8c72f5c/synology_dsm/synology_dsm.py#L264

Yes totally, setting the sensor to off will make the API continue to fetch on it.

Your original approach of the option to turn on/off the sensor does eliminate this issue. The downside is that every new sensor type would have to be added to the options flow which will make it a lot more difficult to refactor later to tie specific sensors to specific api calls to avoid the ones that are disabled.

Not totally, one option per API, not per sensor.

Maybe, at SynoApi.async_update I can check all entities of this config, and if all entities of a specific API are disabled: something like this self.dsm._security = None.

@bdraco Do you know how to get all entities of one config ?

Copy link
Copy Markdown
Member Author

@Quentame Quentame May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this:

    registry = await entity_registry.async_get_registry(hass)
    conditions: List[Dict[str, Any]] = []

    # Get all the integrations entities for this device
    for entry in entity_registry.async_entries_for_device(registry, device_id):

I suppose

EDIT: async_entries_for_config_entry

Copy link
Copy Markdown
Member

@bdraco bdraco May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally in this case we would want each entity to subscribe to updates and then we can look at the list of subscribers and determine which endpoints to poll. august/subscriber.py has an example

}
)
return self.async_show_form(step_id="init", data_schema=data_schema)
Expand Down
7 changes: 7 additions & 0 deletions homeassistant/components/synology_dsm/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
UNDO_UPDATE_LISTENER = "undo_update_listener"

# Configuration
CONF_SECURITY = "security"
CONF_VOLUMES = "volumes"

DEFAULT_SSL = True
DEFAULT_PORT = 5000
DEFAULT_PORT_SSL = 5001
# Options
DEFAULT_SECURITY = True
DEFAULT_SCAN_INTERVAL = 15 # min

UTILISATION_SENSORS = {
Expand Down Expand Up @@ -58,5 +61,9 @@
"disk_temp": ["Temperature", None, "mdi:thermometer"],
}

SECURITY_SENSORS = {
"status": ["Security status", None, "mdi:checkbox-marked-circle-outline"],
}


TEMP_SENSORS_KEYS = ["volume_disk_temp_avg", "volume_disk_temp_max", "disk_temp"]
39 changes: 38 additions & 1 deletion homeassistant/components/synology_dsm/sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
BASE_NAME,
CONF_VOLUMES,
DOMAIN,
SECURITY_SENSORS,
STORAGE_DISK_SENSORS,
STORAGE_VOL_SENSORS,
SYNO_API,
Expand All @@ -42,6 +43,12 @@ async def async_setup_entry(
for sensor_type in UTILISATION_SENSORS
]

if api.security:
sensors += [
SynoNasSecuritySensor(api, sensor_type, SECURITY_SENSORS[sensor_type])
for sensor_type in SECURITY_SENSORS
]

# Handle all volumes
if api.storage.volumes_ids:
for volume in entry.data.get(CONF_VOLUMES, api.storage.volumes_ids):
Expand Down Expand Up @@ -135,7 +142,7 @@ def should_poll(self) -> bool:

async def async_update(self):
"""Only used by the generic entity update service."""
await self._api.update()
await self._api.async_update()

async def async_added_to_hass(self):
"""Register state update callback."""
Expand Down Expand Up @@ -208,3 +215,33 @@ def device_info(self) -> Dict[str, any]:
"sw_version": self._api.information.version_string,
"via_device": (DOMAIN, self._api.information.serial),
}


class SynoNasSecuritySensor(SynoNasSensor):
"""Representation a Synology Security sensor."""

@property
def state(self):
"""Return the state."""
return getattr(self._api.security, self.sensor_type)
Copy link
Copy Markdown
Member Author

@Quentame Quentame May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state here is "safe", but maybe I should create a binary sensor of it instead of a sensor.

Like "Synology security safe" : on if safe, off if not, and maybe a details of it in attrs.

(I don't know all possible values)

What do you think @balloob ?

Copy link
Copy Markdown
Member

@bdraco bdraco May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are spot on and it should be a binary sensor.

Copy link
Copy Markdown
Member

@balloob balloob May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

binary sensor yes. There is even a safety device class :)

https://developers.home-assistant.io/docs/entity_binary_sensor#available-device-classes

Note that off means safe.


async def async_remove(self):
Comment thread
Quentame marked this conversation as resolved.
"""Clean up when removing entity.

Remove entity if no entry in entity registry exist.
Remove entity registry entry if no entry in device registry exist.
Remove entity registry entry if there are more than one entity linked to the device registry entry.
"""
entity_registry = await self.hass.helpers.entity_registry.async_get_registry()
entity_entry = entity_registry.async_get(self.entity_id)
if not entity_entry:
await super().async_remove()
return

device_registry = await self.hass.helpers.device_registry.async_get_registry()
device_entry = device_registry.async_get(entity_entry.device_id)
if not device_entry:
entity_registry.async_remove(self.entity_id)
return

entity_registry.async_remove(self.entity_id)
3 changes: 2 additions & 1 deletion homeassistant/components/synology_dsm/strings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
"step": {
"init": {
"data": {
"scan_interval": "Minutes between scans"
"scan_interval": "Minutes between scans",
"security": "Add security sensors"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion homeassistant/components/synology_dsm/translations/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
"step": {
"init": {
"data": {
"scan_interval": "Minutes between scans"
"scan_interval": "Minutes between scans",
"security": "Add security sensors"
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion tests/components/synology_dsm/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
from homeassistant.components import ssdp
from homeassistant.components.synology_dsm.config_flow import CONF_OTP_CODE
from homeassistant.components.synology_dsm.const import (
CONF_SECURITY,
CONF_VOLUMES,
DEFAULT_PORT,
DEFAULT_PORT_SSL,
DEFAULT_SCAN_INTERVAL,
DEFAULT_SECURITY,
DEFAULT_SSL,
DOMAIN,
)
Expand Down Expand Up @@ -425,11 +427,13 @@ async def test_options_flow(hass: HomeAssistantType, service: MagicMock):
)
assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
assert config_entry.options[CONF_SCAN_INTERVAL] == DEFAULT_SCAN_INTERVAL
assert config_entry.options[CONF_SECURITY] == DEFAULT_SECURITY

# Manual
result = await hass.config_entries.options.async_init(config_entry.entry_id)
result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={CONF_SCAN_INTERVAL: 2},
result["flow_id"], user_input={CONF_SCAN_INTERVAL: 2, CONF_SECURITY: False},
)
assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
assert config_entry.options[CONF_SCAN_INTERVAL] == 2
assert not config_entry.options[CONF_SECURITY]