pgsql-jellyfin/docs/syncplay-authorization-error-handling.md

# 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.