feat(auth): implement native Home Assistant Ingress authentication#544
Open
coaster3000 wants to merge 4 commits into
Open
feat(auth): implement native Home Assistant Ingress authentication#544coaster3000 wants to merge 4 commits into
coaster3000 wants to merge 4 commits into
Conversation
Contributor
WalkthroughAdds configurable Home Assistant ingress authentication: new config flags and schema, docs, a oneshot s6 startup task that injects trusted-subnet and middleware settings into Grocy’s ChangesHome Assistant Ingress Authentication Feature
Infrastructure Dependency Update
Sequence DiagramsequenceDiagram
participant Client
participant GrocyMiddleware as Grocy::HomeAssistantAuthMiddleware
participant Ingress as HomeAssistantIngress
participant Supervisor
participant DB as LocalDatabase/UsersService
Client->>GrocyMiddleware: HTTP request (with HA headers or API key)
GrocyMiddleware->>GrocyMiddleware: Try ApiKeyAuthMiddleware
alt API key valid
GrocyMiddleware->>DB: validate session/user
GrocyMiddleware-->>Client: allow request
else API key invalid
GrocyMiddleware->>Ingress: validate source IP (HA_TRUSTED_SUBNET)
alt from trusted CIDR and X-Remote-User-Name present
GrocyMiddleware->>DB: find or create user by username
GrocyMiddleware-->>Client: create session / allow request
else supervisor login flow
Client->>Supervisor: POST /auth (via ProcessLogin)
Supervisor-->>GrocyMiddleware: 200 / user info
GrocyMiddleware->>DB: ensure local user and create session
GrocyMiddleware-->>Client: set session cookie / allow
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php`:
- Around line 25-46: The HomeAssistantAuthMiddleware currently trusts
X-Remote-User-Name/X-Remote-User-Display-Name unconditionally; before using
$request->getHeaderLine('X-Remote-User-Name') (and before auto-provisioning via
UsersService::GetInstance()->CreateUser), add a strict trusted-proxy check
(e.g., verify remote IP against a configured trusted proxy list or require an
ingress marker header set only by the proxy) and only accept header-based auth
if that check passes; if the request is not from a trusted proxy, skip
header-based auth and fall back to SessionAuthMiddleware->authenticate($request)
(and do not auto-provision users).
- Around line 64-75: The supervisor auth curl call in
HomeAssistantAuthMiddleware.php (the block that initializes $ch and calls
curl_exec to get $response and $httpCode) must be hardened: set
CURLOPT_CONNECTTIMEOUT and CURLOPT_TIMEOUT to reasonable values, check
curl_exec() for false and capture curl_error() to handle network failures, treat
non-2xx $httpCode as an error, and ensure curl_close($ch) is always called
(including on error paths); update the error handling to log/throw a descriptive
error including the curl error or HTTP code so login won't hang on stalled
supervisor calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ac0b60c-8528-480a-a5c1-ab3106fa3ba3
📒 Files selected for processing (10)
grocy/DOCS.mdgrocy/Dockerfilegrocy/config.yamlgrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/dependencies.d/init-grocygrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/rungrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/typegrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/upgrocy/rootfs/etc/s6-overlay/s6-rc.d/php-fpm/dependencies.d/init-ingress-authgrocy/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/init-ingress-authgrocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php
- Add required API permissions and cURL extension to configuration - Implement HomeAssistantAuthMiddleware with graceful Supervisor proxy fallback for direct-port logins - Automate middleware injection via a sequenced S6-overlay v3 oneshot service - Update DOCS.md to detail the new 'enable_ingress_auth' option
coaster3000
force-pushed
the
feature/ha_auth
branch
from
7bff6eb to
703b54b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php (1)
82-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
curl_close($ch)causes resource leak.The previous review suggested adding
curl_close($ch)but it's still missing on all code paths. The handle is never closed after the request completes, regardless of success or failure.Suggested fix
$response = curl_exec($ch); if ($response === false) { + curl_close($ch); return false; } $httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE); + curl_close($ch); if ($httpCode === 200)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php` around lines 82 - 88, The curl handle ($ch) opened in HomeAssistantAuthMiddleware (the code around curl_exec($ch) / curl_getinfo($ch)) is never closed; add curl_close($ch) on all control paths—immediately before returning false when curl_exec returns false, and after the request completes (before continuing or returning) once you’ve read curl_getinfo($ch, CURLINFO_HTTP_CODE); ensure every early return in that method closes $ch so no resource leak remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php`:
- Line 14: The constant GROCY_EXTERNALLY_MANAGED_AUTHENTICATION is being
unconditionally defined in HomeAssistantAuthMiddleware (causing a notice if
authenticate() runs multiple times); guard the definition by checking
defined('GROCY_EXTERNALLY_MANAGED_AUTHENTICATION') first and only call
define('GROCY_EXTERNALLY_MANAGED_AUTHENTICATION', true) if it is not already
defined so repeated calls to authenticate() won’t raise a PHP notice.
- Line 102: The code calls self::SetSessionCookie($sessionKey) but no such
method or parent exists; add a SetSessionCookie(string $sessionKey) method
inside HomeAssistantAuthMiddleware (or make the class extend/import the correct
AuthMiddleware that provides it). Implement SetSessionCookie to write the
session cookie consistently with the app (use the existing cookie helper or PHP
setcookie), setting HttpOnly, Secure, SameSite and appropriate
path/domain/expiration pulled from config/Session settings, and ensure it
accepts the $sessionKey parameter and returns/sets nothing (void) so the call at
line 102 resolves without runtime errors.
---
Duplicate comments:
In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php`:
- Around line 82-88: The curl handle ($ch) opened in HomeAssistantAuthMiddleware
(the code around curl_exec($ch) / curl_getinfo($ch)) is never closed; add
curl_close($ch) on all control paths—immediately before returning false when
curl_exec returns false, and after the request completes (before continuing or
returning) once you’ve read curl_getinfo($ch, CURLINFO_HTTP_CODE); ensure every
early return in that method closes $ch so no resource leak remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d85aa814-cd33-4171-86b8-d454157e32bd
📒 Files selected for processing (10)
grocy/DOCS.mdgrocy/Dockerfilegrocy/config.yamlgrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/dependencies.d/init-grocygrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/rungrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/typegrocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/upgrocy/rootfs/etc/s6-overlay/s6-rc.d/php-fpm/dependencies.d/init-ingress-authgrocy/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/init-ingress-authgrocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php
✅ Files skipped from review due to trivial changes (3)
- grocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/type
- grocy/DOCS.md
- grocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/up
🚧 Files skipped from review as they are similar to previous changes (3)
- grocy/config.yaml
- grocy/Dockerfile
- grocy/rootfs/etc/s6-overlay/s6-rc.d/init-ingress-auth/run
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coaster3000
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grocy/config.yaml (1)
99-99: ⚡ Quick winHarden CIDR validation in schema for
trusted_ingress_subnet.Line 99 currently accepts any string; constraining this to CIDR-like input in schema would catch bad config earlier and avoid runtime auth surprises.
Proposed schema tightening
- trusted_ingress_subnet: str + trusted_ingress_subnet: match(^([0-9]{1,3}\.){3}[0-9]{1,3}\/([0-9]|[1-2][0-9]|3[0-2])$)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grocy/config.yaml` at line 99, The schema entry trusted_ingress_subnet currently allows any string; tighten validation so only CIDR-like values are accepted by replacing the loose str type with a CIDR validator (e.g., a regex that enforces IPv4/IPv6/CIDR format or a proper ip_network validator used by your schema system) and update the schema metadata for trusted_ingress_subnet to reject non-CIDR strings; ensure the validator accepts single-address /32 or /128 masks and ranges like 192.168.0.0/16 and 2001:db8::/32 and returns a clear validation error mentioning trusted_ingress_subnet when invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@grocy/config.yaml`:
- Line 99: The schema entry trusted_ingress_subnet currently allows any string;
tighten validation so only CIDR-like values are accepted by replacing the loose
str type with a CIDR validator (e.g., a regex that enforces IPv4/IPv6/CIDR
format or a proper ip_network validator used by your schema system) and update
the schema metadata for trusted_ingress_subnet to reject non-CIDR strings;
ensure the validator accepts single-address /32 or /128 masks and ranges like
192.168.0.0/16 and 2001:db8::/32 and returns a clear validation error mentioning
trusted_ingress_subnet when invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4427ae9b-cd85-400c-be4f-93ba726fa9c4
📒 Files selected for processing (2)
grocy/DOCS.mdgrocy/config.yaml
✅ Files skipped from review due to trivial changes (1)
- grocy/DOCS.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php (1)
148-161: ⚡ Quick winAdd defensive validation for CIDR format.
While the config schema validates the format, the
HA_TRUSTED_SUBNETconstant could be misconfigured. If$cidrlacks a/or contains an invalid subnet, this method will throw aValueError(PHP 8+ list destructuring) or produce incorrect results fromip2long()returningfalse.🛡️ Proposed defensive fix
private function ipInSubnet(string $ip, string $cidr): bool { if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) === false) { return false; } - list($subnet, $bits) = explode('/', $cidr); + $parts = explode('/', $cidr); + if (count($parts) !== 2) { + return false; + } + list($subnet, $bits) = $parts; + $bits = (int)$bits; + + if ($bits < 0 || $bits > 32) { + return false; + } + + $subnetLong = ip2long($subnet); + if ($subnetLong === false) { + return false; + } + $ipLong = ip2long($ip); - $subnetLong = ip2long($subnet); - $maskLong = ~((1 << (32 - (int)$bits)) - 1); + $maskLong = ~((1 << (32 - $bits)) - 1); return ($ipLong & $maskLong) === ($subnetLong & $maskLong); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php` around lines 148 - 161, The ipInSubnet(string $ip, string $cidr): bool method currently assumes $cidr is a valid "subnet/bits" string and that ip2long returns valid integers; add defensive validation in ipInSubnet: verify $cidr contains a '/' before explode, ensure $bits is numeric and between 0 and 32, validate that $subnet and $ip parse with filter_var(FILTER_FLAG_IPV4) and ip2long() do not return false (for $ip, $subnet and after casting $bits), and return false immediately on any validation failure; only compute $maskLong and perform the bitwise comparison when all parsed values are valid so ipInSubnet and related callers (ipInSubnet) cannot throw ValueError or use false results from ip2long.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@grocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php`:
- Around line 148-161: The ipInSubnet(string $ip, string $cidr): bool method
currently assumes $cidr is a valid "subnet/bits" string and that ip2long returns
valid integers; add defensive validation in ipInSubnet: verify $cidr contains a
'/' before explode, ensure $bits is numeric and between 0 and 32, validate that
$subnet and $ip parse with filter_var(FILTER_FLAG_IPV4) and ip2long() do not
return false (for $ip, $subnet and after casting $bits), and return false
immediately on any validation failure; only compute $maskLong and perform the
bitwise comparison when all parsed values are valid so ipInSubnet and related
callers (ipInSubnet) cannot throw ValueError or use false results from ip2long.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8859195-f261-425b-a612-0c4b842afea8
📒 Files selected for processing (2)
grocy/config.yamlgrocy/rootfs/var/www/grocy/middleware/HomeAssistantAuthMiddleware.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Introduce seamless, native ingress authentication within Grocy via a new configuration option: enable_ingress_auth. When enabled, users are automatically provisioned and logged into Grocy with their active home assistant login via their HA session headers.
Additionally, users are authenticated through HA's supervisor when logging in directly from grocy.
Related Issues
No related issues.
Summary by CodeRabbit
New Features
Configuration
Chores
Documentation