Feat: basic auth support for calendar (optional) #363
Conversation
|
Are we waiting for that person you asked to test this? |
The code should be fine, at least not break anything. Feel free to merge if it looks ok to you :) |
JW-CH
force-pushed
the
dev/jw/calendarauth
branch
from
0caf4a8 to
cecb3b4
Compare
| var appointments = new List<IAppointment>(); | ||
|
|
||
| var icals = await GetCalendars(_serverSettings.Webcalendars); | ||
| List<(string? auth, string url)> cals = _serverSettings.Webcalendars.Select(x => x.Contains(';') ? (x.Split(';')[0], x.Split(';')[1]) : (null, x.ToString())).ToList(); |
There was a problem hiding this comment.
I would suggest that you use the standard auth formatting in URLs: https://<user>:<pass>@host/path, rather that semicolon delimited.
|
I just tried the dev/jw/calendarauth branch and can confirm that using an ics from Baikal CalDAV works fine. Here an example of the env (redacted): Webcalendars="user:password;https://baikal.mydomain.tld/dav.php/calendars/user/default?export" The URL returns directly the .ics file (In case you wonder why there isn't a .ics in the path 😃 ) Looking forward for it to get merged. |
|
@JW-CH Will this be merged into the main branch soon? I haven't shared my Google calendars, but I'd still like to be able to view them 😁 |
JW-CH
force-pushed
the
dev/jw/calendarauth
branch
from
cecb3b4 to
21554bf
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpdated IcalCalendarService to use IHttpClientFactory and ILogger, create per-call HttpClient, support Basic Auth via URL userinfo, convert webcal:// to https://, change GetCalendars to accept (auth, url) tuples, and update docs and example env to show auth-capable calendar URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant IcalService as IcalCalendarService
participant Factory as IHttpClientFactory
participant Endpoint as CalendarEndpoint
participant Logger
Caller->>IcalService: GetCalendars([(auth, url), ...])
loop per calendar
IcalService->>Logger: Debug "loading calendar (url)"
IcalService->>Factory: CreateClient()
Factory-->>IcalService: HttpClient
alt auth provided
IcalService->>IcalService: Add Authorization header (Basic ...)
end
IcalService->>Endpoint: GET https://... (with headers)
alt 200 OK
Endpoint-->>IcalService: 200 + ICS data
else non-200
Endpoint-->>IcalService: Error (status)
IcalService->>Logger: Error "failed to load (url, status)"
end
end
IcalService-->>Caller: Processed calendars/results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/docs/getting-started/configuration.md`:
- Around line 167-168: Under the "### Metadata" heading remove the placeholder
text "Needs documentation" (or replace it with actual documentation for the
Metadata section) so the published docs don't contain the placeholder; locate
the "### Metadata" heading in the docs and either delete the placeholder line or
replace it with the intended explanatory content, examples, and any relevant
fields or configuration details.
In `@ImmichFrame.Core/Services/IcalCalendarService.cs`:
- Around line 50-51: The current debug log prints calendar.auth which contains
credentials; update the log in IcalCalendarService to never include
calendar.auth value and instead log only whether auth is present (e.g.,
"auth=present" or "auth=none") and still include calendar.url for context;
ensure no other logs print calendar.auth (note calendar.auth is later used for
Basic auth in the block around the Authorization header setup/usage), and remove
any other exposures of the raw credential string in _logger calls or exception
messages.
- Around line 12-14: The field declaration for _appointmentCache uses a generic
ApiCache<List<IAppointment>> but ApiCache is non-generic; update the
IcalCalendarService field to use the non-generic ApiCache type (remove the
generic type argument) and adjust the constructor invocation to new
ApiCache(TimeSpan.FromMinutes(15)) so _appointmentCache is declared as ApiCache
_appointmentCache and initialized accordingly; search for references to
_appointmentCache in IcalCalendarService to ensure any code treating it as
generic (e.g., expecting typed getters/setters) is updated to use the
non-generic API.
- Around line 27-29: The parsing of _serverSettings.Webcalendars into cals
currently uses Split(';') (and calls Split twice), which breaks URLs containing
';' and is inefficient; change the logic that builds List<(string? auth, string
url)> cals to for each entry find the first delimiter index via IndexOf(';') and
then use Substring(0, index) for auth and Substring(index + 1) for url (or set
auth = null and url = whole entry when no delimiter), then pass that list to
GetCalendars — this ensures only the first ';' is treated as the auth separator
and avoids multiple Split calls.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Services/IcalCalendarService.cs`:
- Around line 59-60: The HttpRequestMessage and HttpResponseMessage created
inside the loop are not disposed; change the allocations to scoped disposals so
resources are released each iteration—replace the raw new
HttpRequestMessage(...) with a scoped using (e.g., using var request = new
HttpRequestMessage(...)) and likewise wrap the result of SendAsync into a using
(e.g., using var response = await _httpClient.SendAsync(request)) within the
same loop in IcalCalendarService so both HttpRequestMessage and
HttpResponseMessage are disposed after each iteration.
- Around line 55-57: The LogDebug call is logging raw calendar URLs
(calendar.url), which can leak credentials or tokens; update the logging in
IcalCalendarService (the spot using _logger.LogDebug and variables calendar.url,
calendar.auth and httpUrl) to first sanitize the URL by parsing calendar.url and
building a redacted form scheme://host[:port]/path (removing userinfo and
query/fragment) and then use structured logging with named parameters (e.g.,
_logger.LogDebug("Loading calendar: {AuthState} {SanitizedUrl}", authState,
sanitizedUrl)) so no sensitive parts are recorded.
Use standard https://user:pass@host/path format instead of semicolon- delimited auth. Adds error handling for malformed URLs and properly unescapes percent-encoded credentials.
JW-CH
force-pushed
the
dev/jw/calendarauth
branch
from
33376e4 to
99ec031
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Services/IcalCalendarService.cs`:
- Around line 74-77: The Basic auth header construction in IcalCalendarService
uses Encoding.ASCII on calendar.auth which will corrupt non-ASCII credentials;
change Encoding.ASCII to Encoding.UTF8 when getting bytes for
Convert.ToBase64String so the Authorization header
(request.Headers.Authorization set in the method that builds the request) uses
UTF-8 encoding for calendar.auth and supports international characters.
- Around line 80-88: Add per-calendar exception handling around the HTTP call in
the loop that calls client.SendAsync(request) so a failure for one calendar does
not abort processing of the rest: wrap the await client.SendAsync(request) and
subsequent response handling in a try/catch that catches HttpRequestException
and TaskCanceledException (and optionally a general Exception fallback), log the
error via _logger.LogError with httpUrl and the exception, and continue to the
next calendar; also ensure the HttpResponseMessage is disposed by using a using
(or using declaration) around the response variable when reading
response.Content and adding to icals.
♻️ Duplicate comments (3)
docs/docs/getting-started/configuration.md (1)
169-170: Remove the “Needs documentation” placeholder before release.
Leaving placeholders in published docs is confusing.ImmichFrame.Core/Services/IcalCalendarService.cs (2)
43-44: Sanitize calendar URLs in logs to avoid leaking secrets.
Raw URLs (and invalid inputs) may include credentials or tokens. Log only a redacted scheme/host/path and use structured logging.🔧 Proposed fix (sanitized structured logging)
- _logger.LogError($"Invalid calendar URL: '{x}'"); + _logger.LogError("Invalid calendar URL: {CalendarUrl}", SanitizeCalendarUrl(x));- _logger.LogDebug($"Loading calendar: {(calendar.auth != null ? "[authenticated]" : "no auth")} - {calendar.url}"); + var authMode = string.IsNullOrEmpty(calendar.auth) ? "no auth" : "basic auth"; + var safeUrl = SanitizeCalendarUrl(httpUrl); + _logger.LogDebug("Loading calendar: {AuthMode} - {CalendarUrl}", authMode, safeUrl); ... - _logger.LogError($"Failed to load calendar data from '{httpUrl}' (Status: {response.StatusCode})"); + _logger.LogError("Failed to load calendar data from {CalendarUrl} (Status: {StatusCode})", safeUrl, response.StatusCode);// Add helper in the class (outside methods) private static string SanitizeCalendarUrl(string? raw) { if (string.IsNullOrWhiteSpace(raw)) return "[empty-url]"; return Uri.TryCreate(raw, UriKind.Absolute, out var uri) ? $"{uri.Scheme}://{uri.Host}{(uri.IsDefaultPort ? "" : $":{uri.Port}")}{uri.AbsolutePath}" : "[invalid-url]"; }Also applies to: 68-88
72-83: Dispose HTTP request/response objects to prevent resource leaks.
BothHttpRequestMessageandHttpResponseMessageareIDisposableand should be scoped.🔧 Proposed fix
- var request = new HttpRequestMessage(HttpMethod.Get, httpUrl); + using var request = new HttpRequestMessage(HttpMethod.Get, httpUrl); ... - HttpResponseMessage response = await client.SendAsync(request); + using var response = await client.SendAsync(request);
🧹 Nitpick comments (1)
docs/docs/getting-started/configuration.md (1)
164-168: Add a note to URL‑encode special characters in credentials.
If usernames/passwords include@,:, or spaces, they must be percent‑encoded to keep the URL parseable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/docs/getting-started/configuration.md`:
- Around line 164-167: Replace the misleading "With Auth" Google Calendar
example that shows basic auth in the URL
(https://username:password@calendar.google.com/...) with either a
provider-agnostic Basic Auth example (e.g.,
https://username:password@example.com/path.ics) or add a caveat next to the
Google Calendar example stating that Google Calendar does not support basic auth
in ICS URLs and requires unauthenticated secret-token URLs or OAuth; update the
example text near the "No Auth" / "With Auth" lines so it clearly distinguishes
provider support and links auth behavior to provider-specific requirements.
@Buddinski88 Unfortunately, Google Calendar does not support Basic Authentication for .ics feeds. This means even when this PR gets merged and enables Basic Auth for calendar URLs, it won't work with Google Calendar. Google Calendar provides "secret" .ics URLs that work without authentication (they're not publicly shared, but anyone with the link can access them) |
|
@JW-CH did you test that with Google? I used the “secret” .ics link this morning and it didn't work to load the calendar. There should be several calendars possible, right? Settings.yml General:
AuthenticationSecret: 'qC1Y9nicjLGgXkEM9A1kQdxqIA2waRAtORNma2VhvM'
RenewImagesDuration: 30
Webcalendars:
- https://calendar.google.com/calendar/ical/**group.calendar.google.com/private-***/basic.ics
- https://calendar.google.com/calendar/ical/**group.calendar.google.com/private-***/basic.ics
- https://calendar.google.com/calendar/ical/**group.calendar.google.com/private-***/basic.ics
... |
|
yes, this is working for me: Webcalendars:
- https://calendar.google.com/calendar/ical/mymail%40gmail.com/private-xxxxxxxxxxxxxxxxxxxxxxxx/basic.icsLooks like you are using a group-calendar? |
|
@JW-CH exactly. I use a total of five calendars, which I have shared with my wife, so probably group calendars as well. I assume that's not supported yet? 😁 |
Closes #281
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.