Repository navigation
Adds new integration [wallenium/HA-menstrual-cycle] - #9845
Conversation
|
Thank you for submitting your repository to HACS (Home Assistant Community Store). Your submission is in the review queue: You can view the current queue here. Pull requests are processed in the order they were created, oldest first. What to avoid during review:
If you need to make changes: About draft pull requests: What happens next:
|
|
Thank you for submitting your repository to HACS (Home Assistant Community Store). Your submission is in the review queue: You can view the current queue here. Pull requests are processed in the order they were created, oldest first. What to avoid during review:
If you need to make changes: About draft pull requests: What happens next:
|
frenck
left a comment
There was a problem hiding this comment.
Hi @wallenium, thanks for the submission! 👋
Two things need fixing before this can go in:
-
The ICS feed URL, including its token, is published in state attributes.
sensor.py(line 1440) putsics_url: /menstruation_cycle/ics/<token>.icsinto the main profile sensor's attributes, and at the default visibility level that attribute is always there. That means every Home Assistant user (including non-admins, and the sidebar panel is registered withrequire_admin=False) can read it, and the recorder writes it to the database on every state change. The feed itself is served by a raw route that bypasses Home Assistant authentication (__init__.py, lines 3880-3962), is enabled for every profile without the user asking for it, ignores the profile's visibility level (a profile switched toprivatekeeps serving its period and fertility predictions to anyone with an old URL), and is not mentioned in the README. For data this sensitive, please:- stop exposing the URL in state attributes;
- make the feed opt-in per profile (off by default) and respect the
privatevisibility level; - only hand the URL to admins, for example by showing it in the options flow or returning it from an admin-only service or websocket command, which the dashboard panel can then use instead of reading the attribute;
- document in the README that the feed works without a login, so anyone with the URL can read the predictions, and how to rotate it.
-
Blocking I/O on the event loop.
async_get_config_entry_diagnosticsindiagnostics.py(line 54) calls_load_manifest_version(), which opensmanifest.jsonsynchronously (__init__.py, lines 230-237). Home Assistant flags that as a blocking call inside the event loop. You already have the value in the module-levelRESOURCE_VERSION, so just use that. The same applies, on a smaller scale, to thePath.is_file()checks in the async HTTP handlers (__init__.py, lines 3810, 3832 and 3851) and inimage.py(line 152, reached fromasync_update). Move those into the executor job that already reads the file, or drop them and handleFileNotFoundErrorfrom the read instead.
Not a blocker, just a suggestion: the dashboard panel loads Google Fonts at runtime (www/menstruation-cycle-dashboard-panel.js, line 6056), so every time someone opens a health-data panel, their browser contacts Google. Bundling the fonts or using the Home Assistant theme fonts would keep it fully local.
Once that's addressed, push a new release and re-request review.
../Frenck
Blogging my personal ramblings at frenck.dev
Or subscribe to my free bi-weekly newsletter: frenck.dev/newsletter
|
Hi @frenck |
frenck
left a comment
There was a problem hiding this comment.
Hi @wallenium, thanks for the quick turnaround on 1.9.0! 👋
One part of the ICS item from the last round is still open: there is no way to rotate the token when you need to.
_async_rotate_ics_token (__init__.py, lines 2507-2525) is only reachable from the stale-token repair flow (repairs.py, lines 1089-1117), and that issue only appears once the token is 365 days old (ICS_TOKEN_STALE_DAYS in const.py, line 554). Turning the feed off and on again does not create a new token either, so the old URL just starts working again. That causes two problems:
- Someone who shared the URL and wants to revoke it can only switch the feed off for good. "Use the repair issue" (README, line 108) doesn't help for the first year.
- On upgrade,
async_setup_entrykeeps the token that is already stored (__init__.py, lines 3292-3305). In 1.8.0 and earlier, that exact token was in theics_urlstate attribute, readable by every user and written to the recorder. As soon as an admin turns the new opt-in feed on, that old URL is live again.
Please add an on-demand rotation (for example a "generate a new feed URL" checkbox in the options flow, or an admin-only service or websocket command the dashboard can call), and generate a fresh token when the feed is switched from off to on, so tokens from older releases never come back to life. Then update the README section on how to invalidate a URL.
Not a blocker, just a suggestion: the new menstruation_cycle_pill_taken, menstruation_cycle_lh_positive and menstruation_cycle_ovulation_confirmed events are fired (and shown in the logbook for every user) at the "Status only" visibility level too (__init__.py, lines 5339-5368, and _fire_ovulation_confirmed in sensor.py, lines 1505-1533). The options flow says "Status only" hides health details from others, so you may want to skip those events unless the level is "Full".
Once that's addressed, push a new release and re-request review.
../Frenck
Blogging my personal ramblings at frenck.dev
Or subscribe to my free bi-weekly newsletter: frenck.dev/newsletter
- **ICS feed token:** every time the feed is switched from off to on, a new token is created, so URLs from earlier versions (when the token was published as the `ics_url` attribute) or from an earlier period of use never work again. Administrators can also rotate the token at any time with the new "New feed URL" button next to the "Subscribe to calendar" link in the dashboard (admin-only websocket command `menstruation_cycle/rotate_ics_token`); the old URL stops working immediately. The README explains how to invalidate a URL. - The events `menstruation_cycle_pill_taken`, `menstruation_cycle_lh_positive` and `menstruation_cycle_ovulation_confirmed` (and so their logbook entries) are only fired at visibility "Full" instead of "Full" and "Status only", because the logbook is readable by every user. Automations that rely on these events for a "Status only" profile need visibility "Full". refer to hacs/default#9845 (review)
|
Hi @frenck
please check again if I fixed all regressions. Simon |
frenck
left a comment
There was a problem hiding this comment.
Hi @wallenium, thanks for the quick turnaround on 1.9.1 and the on-demand feed rotation! 👋
One small thing for a future release: two user-facing texts in strings.json and translations/*.json still describe the old behavior. The ics_feed_enabled option description still says to invalidate a shared URL through the repair issue or by turning the feed off, without the new "New feed URL" button, and the set_profile_visibility service description still says the read services are not affected by the visibility level, which 1.9.1 changed. Home Assistant shows these translations rather than services.yaml, so that is the text people read.
Approving and merging now.
../Frenck
Blogging my personal ramblings at frenck.dev
Or subscribe to my free bi-weekly newsletter: frenck.dev/newsletter
|
Congratulations! Your repository has been added to HACS 🎉 Your repository has been approved: Resources: Thank you for contributing to the Home Assistant community! 🎉 |
- Two user-facing texts (`strings.json` and `translations/*.json`) still described the old behavior: the description of the option "Public ICS calendar feed" now mentions the dashboard button "New feed URL" for invalidating a shared URL, and the description of the service `set_profile_visibility` now says that below visibility "Full" the services that return a profile's data answer administrators only (as in `services.yaml`). hacs/default#9845
Checklist
Links
Link to current release: <Release 1.5.3>
Link to successful HACS action (without the
ignorekey): <HACS Action for 1.5.3>Link to successful hassfest action (if integration): <Hassfest Action for 1.5.3>