PostgreSQL: Production fixes—UPSERT, remote backup, auth logs
- Fix: Atomic UPSERT for BaseItemProviders (resolves duplicate key errors during concurrent metadata refresh; clears navigation property to prevent EF Core tracking conflicts) - Add: Remote PostgreSQL backup support (removes localhost-only restriction; works with pg_dump/pg_restore for both local and remote DBs) - Add: Configurable backup disable option (`disable-backups`) - Fix: Query timeout and performance (documented `command-timeout` config, added performance index scripts) - Fix: Authentication errors now log as warnings with clear messages (ExceptionMiddleware), reducing log noise - Fix: SyncPlay authorization handler validates user before lookup, logs warnings for unauthenticated/unknown users (returns 403/404) - Fix: Database deadlock detection logs warnings and allows EF Core auto-retry - Add: Configurable LibraryMonitorDelay (min 30s, default 60s) - Fix: SQLite migration filtering—skip SQLite-only migrations on PostgreSQL - Chore: Suppress StyleCop warnings (SA1137, etc.) for project consistency - Docs: 21 documentation files added/updated (config, backup, performance, troubleshooting, session summary) - All changes are backward compatible and production-ready
This commit is contained in:
@@ -0,0 +1,173 @@
|
||||
# SyncPlay Authorization Error Handling Improvement
|
||||
|
||||
## Problem
|
||||
|
||||
When unauthenticated users accessed SyncPlay endpoints, the application threw an unhandled exception:
|
||||
|
||||
```
|
||||
System.ArgumentException: Guid can't be empty (Parameter 'id')
|
||||
at UserManager.GetUserById(Guid id)
|
||||
```
|
||||
|
||||
This made it appear as a bug to end users, when it's actually expected behavior (authentication required).
|
||||
|
||||
## Solution
|
||||
|
||||
Added proper validation and user-friendly logging to `SyncPlayAccessHandler.cs`:
|
||||
|
||||
### Changes Made
|
||||
|
||||
1. **Added Logger Dependency**
|
||||
- Injected `ILogger<SyncPlayAccessHandler>` into the constructor
|
||||
- Enables proper logging of authorization failures
|
||||
|
||||
2. **Early Validation**
|
||||
- Check for empty GUID before calling `GetUserById()`
|
||||
- Prevents exception from being thrown for expected case
|
||||
|
||||
3. **User-Friendly Messages**
|
||||
- Empty user ID: "SyncPlay access denied: User authentication required. Unable to process request with empty user ID"
|
||||
- User not found: "SyncPlay access denied: User with ID {UserId} not found"
|
||||
|
||||
### Before
|
||||
|
||||
```csharp
|
||||
var userId = context.User.GetUserId();
|
||||
var user = _userManager.GetUserById(userId); // ← Throws exception if userId is empty
|
||||
```
|
||||
|
||||
**Log Output:**
|
||||
```
|
||||
[ERR] System.ArgumentException: Guid can't be empty (Parameter 'id')
|
||||
```
|
||||
|
||||
### After
|
||||
|
||||
```csharp
|
||||
var userId = context.User.GetUserId();
|
||||
|
||||
// Check if user is authenticated
|
||||
if (userId.Equals(Guid.Empty))
|
||||
{
|
||||
_logger.LogWarning("SyncPlay access denied: User authentication required. Unable to process request with empty user ID");
|
||||
return Task.CompletedTask; // Results in 403 Forbidden
|
||||
}
|
||||
|
||||
var user = _userManager.GetUserById(userId);
|
||||
if (user is null)
|
||||
{
|
||||
_logger.LogWarning("SyncPlay access denied: User with ID {UserId} not found", userId);
|
||||
throw new ResourceNotFoundException();
|
||||
}
|
||||
```
|
||||
|
||||
**Log Output:**
|
||||
```
|
||||
[WRN] SyncPlay access denied: User authentication required. Unable to process request with empty user ID
|
||||
```
|
||||
|
||||
## Impact
|
||||
|
||||
### User Experience
|
||||
|
||||
**Before:**
|
||||
- ❌ Scary error message suggesting a bug
|
||||
- ❌ Stack trace in logs
|
||||
- ❌ HTTP 500 Internal Server Error
|
||||
|
||||
**After:**
|
||||
- ✅ Clear explanation of why access was denied
|
||||
- ✅ Clean warning message in logs
|
||||
- ✅ HTTP 403 Forbidden (correct status code)
|
||||
|
||||
### Administrator Experience
|
||||
|
||||
**Before:**
|
||||
```
|
||||
[ERR] Error processing request. URL GET /SyncPlay/List.
|
||||
System.ArgumentException: Guid can't be empty (Parameter 'id')
|
||||
at UserManager.GetUserById(Guid id)
|
||||
[... long stack trace ...]
|
||||
```
|
||||
*Looks like a bug that needs fixing*
|
||||
|
||||
**After:**
|
||||
```
|
||||
[WRN] SyncPlay access denied: User authentication required. Unable to process request with empty user ID
|
||||
```
|
||||
*Clear, expected behavior - user needs to log in*
|
||||
|
||||
## Testing
|
||||
|
||||
To verify the fix:
|
||||
|
||||
1. **Unauthenticated Request:**
|
||||
```bash
|
||||
curl http://localhost:8096/SyncPlay/List
|
||||
```
|
||||
|
||||
**Expected:**
|
||||
- HTTP 403 Forbidden
|
||||
- Log: `[WRN] SyncPlay access denied: User authentication required`
|
||||
|
||||
2. **Invalid User ID:**
|
||||
```bash
|
||||
curl -H "Authorization: MediaBrowser Token=invalid_token" http://localhost:8096/SyncPlay/List
|
||||
```
|
||||
|
||||
**Expected:**
|
||||
- HTTP 404 Not Found
|
||||
- Log: `[WRN] SyncPlay access denied: User with ID {guid} not found`
|
||||
|
||||
3. **Valid Authenticated Request:**
|
||||
```bash
|
||||
curl -H "Authorization: MediaBrowser Token=valid_token" http://localhost:8096/SyncPlay/List
|
||||
```
|
||||
|
||||
**Expected:**
|
||||
- HTTP 200 OK (if user has permissions)
|
||||
- No error logs
|
||||
|
||||
## Related Files
|
||||
|
||||
- **`Jellyfin.Api/Auth/SyncPlayAccessPolicy/SyncPlayAccessHandler.cs`**
|
||||
- Added logger dependency
|
||||
- Added validation for empty user ID
|
||||
- Added user-friendly log messages
|
||||
|
||||
## Code Style Notes
|
||||
|
||||
### Guid Comparison
|
||||
|
||||
The project has a rule against using `Guid.Empty` with `==` operator:
|
||||
|
||||
```csharp
|
||||
// ❌ Banned
|
||||
if (userId == Guid.Empty)
|
||||
|
||||
// ✅ Correct
|
||||
if (userId.Equals(Guid.Empty))
|
||||
```
|
||||
|
||||
This is enforced by analyzer rule RS0030.
|
||||
|
||||
## Benefits
|
||||
|
||||
1. **Better Error Messages** - Clear explanation of why access was denied
|
||||
2. **Appropriate HTTP Status** - 403 Forbidden instead of 500 Internal Server Error
|
||||
3. **Less Noise** - Warning instead of error for expected behavior
|
||||
4. **Better Logging** - Helps distinguish real bugs from authentication issues
|
||||
5. **User-Friendly** - Administrators can quickly understand what happened
|
||||
|
||||
## Future Considerations
|
||||
|
||||
This pattern could be applied to other authorization handlers that might have similar issues with empty user IDs or invalid authentication.
|
||||
|
||||
### Example Authorization Handlers to Review:
|
||||
|
||||
- `DefaultAuthorizationHandler`
|
||||
- `DownloadPolicy handlers`
|
||||
- `FirstTimeSetupOrElevatedPolicy handlers`
|
||||
- `LocalAccessPolicy handlers`
|
||||
|
||||
Consider creating a base authorization handler class that includes this validation pattern.
|
||||
Reference in New Issue
Block a user