fix(permission): address code review feedback
- Fix race condition on dialog field with sync.RWMutex protection - Fix double Unref bug in allow/deny callbacks (remove Unref from early return) - Make isAutoAllowSet a standalone function (remove dead code suppression) - Remove unused PermissionTypeMicrophoneAndCamera constant - Move ExtractOrigin to domain/url package (shared utility) - Fix documentation error about origin sharing (github.com vs gist.github.com) - Add test for nil dialog presenter scenario - Update all references to use shared ExtractOrigin
- commit
- 07bdb7cbcd7752cad4bb077b36c51f96b9a1836a
- author
- brice · · 2026-01-30 05:09 UTC
- committer
- brice · · 2026-02-07 11:58 UTC
- parent
- a2a281e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
9 files · +116 -96
M docs/reference/permissions.md +5 -4
| @@ -17,7 +17,8 @@ These permissions require explicit user approval: | |||
| 17 | 17 | ||
| 18 | 18 | - **Microphone** - Websites can request audio capture access | |
| 19 | 19 | - **Camera** - Websites can request video capture access | |
| 20 | - | - **Microphone + Camera** - Combined requests for both audio and video | |
| 20 | + | ||
| 21 | + | When both microphone and camera are requested together, a combined dialog is shown. | |
| 21 | 22 | ||
| 22 | 23 | ## Permission Dialog | |
| 23 | 24 | ||
| @@ -34,10 +35,10 @@ When a site requests mic/camera access, a dialog appears with four options: | |||
| 34 | 35 | ||
| 35 | 36 | ### Origin-Based Storage | |
| 36 | 37 | ||
| 37 | - | Permissions are stored per-origin, not per-page. This means: | |
| 38 | + | Permissions are stored per-origin (scheme + host), not per-page. This means: | |
| 38 | 39 | ||
| 39 | - | - `https://meet.google.com/room1` and `https://meet.google.com/room2` share the same permission | |
| 40 | - | - `https://github.com` and `https://gist.github.com` share the same permission | |
| 40 | + | - `https://meet.google.com/room1` and `https://meet.google.com/room2` share the same permission (same host) | |
| 41 | + | - `https://github.com` and `https://gist.github.com` have different permissions (different hosts) | |
| 41 | 42 | ||
| 42 | 43 | ### Permission States | |
| 43 | 44 | ||
| @@ -17,7 +17,8 @@ These permissions require explicit user approval: | |||||
| 17 | 17 | ||||
| 18 | - **Microphone** - Websites can request audio capture access | 18 | - **Microphone** - Websites can request audio capture access | ||
| 19 | - **Camera** - Websites can request video capture access | 19 | - **Camera** - Websites can request video capture access | ||
| 20 | - | - **Microphone + Camera** - Combined requests for both audio and video | 20 | + | |
| 21 | + | When both microphone and camera are requested together, a combined dialog is shown. | |||
| 21 | 22 | ||||
| 22 | ## Permission Dialog | 23 | ## Permission Dialog | ||
| 23 | 24 | ||||
| @@ -34,10 +35,10 @@ When a site requests mic/camera access, a dialog appears with four options: | |||||
| 34 | 35 | ||||
| 35 | ### Origin-Based Storage | 36 | ### Origin-Based Storage | ||
| 36 | 37 | ||||
| 37 | - | Permissions are stored per-origin, not per-page. This means: | 38 | + | Permissions are stored per-origin (scheme + host), not per-page. This means: |
| 38 | 39 | ||||
| 39 | - | - `https://meet.google.com/room1` and `https://meet.google.com/room2` share the same permission | 40 | + | - `https://meet.google.com/room1` and `https://meet.google.com/room2` share the same permission (same host) |
| 40 | - | - `https://github.com` and `https://gist.github.com` share the same permission | 41 | + | - `https://github.com` and `https://gist.github.com` have different permissions (different hosts) |
| 41 | 42 | ||||
| 42 | ### Permission States | 43 | ### Permission States | ||
| 43 | 44 | ||||
M internal/application/usecase/handle_permission.go +17 -24
| @@ -3,8 +3,7 @@ package usecase | |||
| 3 | 3 | ||
| 4 | 4 | import ( | |
| 5 | 5 | "context" | |
| 6 | - | "fmt" | |
| 7 | - | "net/url" | |
| 6 | + | "sync" | |
| 8 | 7 | "time" | |
| 9 | 8 | ||
| 10 | 9 | "github.com/bnema/dumber/internal/application/port" | |
| @@ -27,6 +26,7 @@ type PermissionCallback struct { | |||
| 27 | 26 | type HandlePermissionUseCase struct { | |
| 28 | 27 | permRepo repository.PermissionRepository | |
| 29 | 28 | dialog port.PermissionDialogPresenter | |
| 29 | + | dialogMu sync.RWMutex | |
| 30 | 30 | } | |
| 31 | 31 | ||
| 32 | 32 | // NewHandlePermissionUseCase creates a new permission handling use case. | |
| @@ -43,9 +43,18 @@ func NewHandlePermissionUseCase( | |||
| 43 | 43 | // SetDialogPresenter sets the dialog presenter. This can be called after | |
| 44 | 44 | // initialization when the UI window is available. | |
| 45 | 45 | func (uc *HandlePermissionUseCase) SetDialogPresenter(dialog port.PermissionDialogPresenter) { | |
| 46 | + | uc.dialogMu.Lock() | |
| 47 | + | defer uc.dialogMu.Unlock() | |
| 46 | 48 | uc.dialog = dialog | |
| 47 | 49 | } | |
| 48 | 50 | ||
| 51 | + | // getDialog safely returns the current dialog presenter. | |
| 52 | + | func (uc *HandlePermissionUseCase) getDialog() port.PermissionDialogPresenter { | |
| 53 | + | uc.dialogMu.RLock() | |
| 54 | + | defer uc.dialogMu.RUnlock() | |
| 55 | + | return uc.dialog | |
| 56 | + | } | |
| 57 | + | ||
| 49 | 58 | // HandlePermissionRequest processes a permission request from WebKit. | |
| 50 | 59 | // This is the main entry point for the permission use case. | |
| 51 | 60 | // | |
| @@ -74,7 +83,7 @@ func (uc *HandlePermissionUseCase) HandlePermissionRequest( | |||
| 74 | 83 | } | |
| 75 | 84 | ||
| 76 | 85 | // Check if all types are auto-allow | |
| 77 | - | if uc.isAutoAllowSet(permTypes) { | |
| 86 | + | if isAutoAllowSet(permTypes) { | |
| 78 | 87 | log.Debug().Msg("auto-allowing permission request") | |
| 79 | 88 | callback.Allow() | |
| 80 | 89 | return | |
| @@ -135,8 +144,7 @@ func (uc *HandlePermissionUseCase) QueryPermissionState( | |||
| 135 | 144 | } | |
| 136 | 145 | ||
| 137 | 146 | // isAutoAllowSet returns true if all permission types in the set are auto-allow. | |
| 138 | - | func (uc *HandlePermissionUseCase) isAutoAllowSet(permTypes []entity.PermissionType) bool { | |
| 139 | - | _ = uc // suppress unused receiver warning, method kept for consistency | |
| 147 | + | func isAutoAllowSet(permTypes []entity.PermissionType) bool { | |
| 140 | 148 | for _, pt := range permTypes { | |
| 141 | 149 | if !entity.IsAutoAllow(pt) { | |
| 142 | 150 | return false | |
| @@ -209,14 +217,16 @@ func (uc *HandlePermissionUseCase) showPermissionDialog( | |||
| 209 | 217 | ) { | |
| 210 | 218 | log := logging.FromContext(ctx) | |
| 211 | 219 | ||
| 220 | + | dialog := uc.getDialog() | |
| 221 | + | ||
| 212 | 222 | // If no dialog presenter is set, deny the permission | |
| 213 | - | if uc.dialog == nil { | |
| 223 | + | if dialog == nil { | |
| 214 | 224 | log.Warn().Str("origin", origin).Msg("no dialog presenter available, denying permission") | |
| 215 | 225 | callback.Deny() | |
| 216 | 226 | return | |
| 217 | 227 | } | |
| 218 | 228 | ||
| 219 | - | uc.dialog.ShowPermissionDialog(ctx, origin, permTypes, func(result port.PermissionDialogResult) { | |
| 229 | + | dialog.ShowPermissionDialog(ctx, origin, permTypes, func(result port.PermissionDialogResult) { | |
| 220 | 230 | if result.Allowed { | |
| 221 | 231 | log.Debug().Bool("persistent", result.Persistent).Msg("user allowed permission") | |
| 222 | 232 | callback.Allow() | |
| @@ -268,20 +278,3 @@ func (uc *HandlePermissionUseCase) persistPermission( | |||
| 268 | 278 | } | |
| 269 | 279 | } | |
| 270 | 280 | } | |
| 271 | - | ||
| 272 | - | // ExtractOrigin extracts the origin (scheme://host) from a URI. | |
| 273 | - | // This normalizes URIs to origins for permission storage. | |
| 274 | - | func ExtractOrigin(uri string) (string, error) { | |
| 275 | - | u, err := url.Parse(uri) | |
| 276 | - | if err != nil { | |
| 277 | - | return "", fmt.Errorf("invalid URI: %w", err) | |
| 278 | - | } | |
| 279 | - | ||
| 280 | - | if u.Scheme == "" || u.Host == "" { | |
| 281 | - | return "", fmt.Errorf("URI missing scheme or host: %s", uri) | |
| 282 | - | } | |
| 283 | - | ||
| 284 | - | // Return scheme://host (without path, query, fragment) | |
| 285 | - | origin := fmt.Sprintf("%s://%s", u.Scheme, u.Host) | |
| 286 | - | return origin, nil | |
| 287 | - | } | |
| @@ -3,8 +3,7 @@ package usecase | |||||
| 3 | 3 | ||||
| 4 | import ( | 4 | import ( | ||
| 5 | "context" | 5 | "context" | ||
| 6 | - | "fmt" | 6 | + | "sync" |
| 7 | - | "net/url" | |||
| 8 | "time" | 7 | "time" | ||
| 9 | 8 | ||||
| 10 | "github.com/bnema/dumber/internal/application/port" | 9 | "github.com/bnema/dumber/internal/application/port" | ||
| @@ -27,6 +26,7 @@ type PermissionCallback struct { | |||||
| 27 | type HandlePermissionUseCase struct { | 26 | type HandlePermissionUseCase struct { | ||
| 28 | permRepo repository.PermissionRepository | 27 | permRepo repository.PermissionRepository | ||
| 29 | dialog port.PermissionDialogPresenter | 28 | dialog port.PermissionDialogPresenter | ||
| 29 | + | dialogMu sync.RWMutex | |||
| 30 | } | 30 | } | ||
| 31 | 31 | ||||
| 32 | // NewHandlePermissionUseCase creates a new permission handling use case. | 32 | // NewHandlePermissionUseCase creates a new permission handling use case. | ||
| @@ -43,9 +43,18 @@ func NewHandlePermissionUseCase( | |||||
| 43 | // SetDialogPresenter sets the dialog presenter. This can be called after | 43 | // SetDialogPresenter sets the dialog presenter. This can be called after | ||
| 44 | // initialization when the UI window is available. | 44 | // initialization when the UI window is available. | ||
| 45 | func (uc *HandlePermissionUseCase) SetDialogPresenter(dialog port.PermissionDialogPresenter) { | 45 | func (uc *HandlePermissionUseCase) SetDialogPresenter(dialog port.PermissionDialogPresenter) { | ||
| 46 | + | uc.dialogMu.Lock() | |||
| 47 | + | defer uc.dialogMu.Unlock() | |||
| 46 | uc.dialog = dialog | 48 | uc.dialog = dialog | ||
| 47 | } | 49 | } | ||
| 48 | 50 | ||||
| 51 | + | // getDialog safely returns the current dialog presenter. | |||
| 52 | + | func (uc *HandlePermissionUseCase) getDialog() port.PermissionDialogPresenter { | |||
| 53 | + | uc.dialogMu.RLock() | |||
| 54 | + | defer uc.dialogMu.RUnlock() | |||
| 55 | + | return uc.dialog | |||
| 56 | + | } | |||
| 57 | + | ||||
| 49 | // HandlePermissionRequest processes a permission request from WebKit. | 58 | // HandlePermissionRequest processes a permission request from WebKit. | ||
| 50 | // This is the main entry point for the permission use case. | 59 | // This is the main entry point for the permission use case. | ||
| 51 | // | 60 | // | ||
| @@ -74,7 +83,7 @@ func (uc *HandlePermissionUseCase) HandlePermissionRequest( | |||||
| 74 | } | 83 | } | ||
| 75 | 84 | ||||
| 76 | // Check if all types are auto-allow | 85 | // Check if all types are auto-allow | ||
| 77 | - | if uc.isAutoAllowSet(permTypes) { | 86 | + | if isAutoAllowSet(permTypes) { |
| 78 | log.Debug().Msg("auto-allowing permission request") | 87 | log.Debug().Msg("auto-allowing permission request") | ||
| 79 | callback.Allow() | 88 | callback.Allow() | ||
| 80 | return | 89 | return | ||
| @@ -135,8 +144,7 @@ func (uc *HandlePermissionUseCase) QueryPermissionState( | |||||
| 135 | } | 144 | } | ||
| 136 | 145 | ||||
| 137 | // isAutoAllowSet returns true if all permission types in the set are auto-allow. | 146 | // isAutoAllowSet returns true if all permission types in the set are auto-allow. | ||
| 138 | - | func (uc *HandlePermissionUseCase) isAutoAllowSet(permTypes []entity.PermissionType) bool { | 147 | + | func isAutoAllowSet(permTypes []entity.PermissionType) bool { |
| 139 | - | _ = uc // suppress unused receiver warning, method kept for consistency | |||
| 140 | for _, pt := range permTypes { | 148 | for _, pt := range permTypes { | ||
| 141 | if !entity.IsAutoAllow(pt) { | 149 | if !entity.IsAutoAllow(pt) { | ||
| 142 | return false | 150 | return false | ||
| @@ -209,14 +217,16 @@ func (uc *HandlePermissionUseCase) showPermissionDialog( | |||||
| 209 | ) { | 217 | ) { | ||
| 210 | log := logging.FromContext(ctx) | 218 | log := logging.FromContext(ctx) | ||
| 211 | 219 | ||||
| 220 | + | dialog := uc.getDialog() | |||
| 221 | + | ||||
| 212 | // If no dialog presenter is set, deny the permission | 222 | // If no dialog presenter is set, deny the permission | ||
| 213 | - | if uc.dialog == nil { | 223 | + | if dialog == nil { |
| 214 | log.Warn().Str("origin", origin).Msg("no dialog presenter available, denying permission") | 224 | log.Warn().Str("origin", origin).Msg("no dialog presenter available, denying permission") | ||
| 215 | callback.Deny() | 225 | callback.Deny() | ||
| 216 | return | 226 | return | ||
| 217 | } | 227 | } | ||
| 218 | 228 | ||||
| 219 | - | uc.dialog.ShowPermissionDialog(ctx, origin, permTypes, func(result port.PermissionDialogResult) { | 229 | + | dialog.ShowPermissionDialog(ctx, origin, permTypes, func(result port.PermissionDialogResult) { |
| 220 | if result.Allowed { | 230 | if result.Allowed { | ||
| 221 | log.Debug().Bool("persistent", result.Persistent).Msg("user allowed permission") | 231 | log.Debug().Bool("persistent", result.Persistent).Msg("user allowed permission") | ||
| 222 | callback.Allow() | 232 | callback.Allow() | ||
| @@ -268,20 +278,3 @@ func (uc *HandlePermissionUseCase) persistPermission( | |||||
| 268 | } | 278 | } | ||
| 269 | } | 279 | } | ||
| 270 | } | 280 | } | ||
| 271 | - | ||||
| 272 | - | // ExtractOrigin extracts the origin (scheme://host) from a URI. | |||
| 273 | - | // This normalizes URIs to origins for permission storage. | |||
| 274 | - | func ExtractOrigin(uri string) (string, error) { | |||
| 275 | - | u, err := url.Parse(uri) | |||
| 276 | - | if err != nil { | |||
| 277 | - | return "", fmt.Errorf("invalid URI: %w", err) | |||
| 278 | - | } | |||
| 279 | - | ||||
| 280 | - | if u.Scheme == "" || u.Host == "" { | |||
| 281 | - | return "", fmt.Errorf("URI missing scheme or host: %s", uri) | |||
| 282 | - | } | |||
| 283 | - | ||||
| 284 | - | // Return scheme://host (without path, query, fragment) | |||
| 285 | - | origin := fmt.Sprintf("%s://%s", u.Scheme, u.Host) | |||
| 286 | - | return origin, nil | |||
| 287 | - | } | |||
M internal/application/usecase/handle_permission_test.go +18 -46
| @@ -12,7 +12,6 @@ import ( | |||
| 12 | 12 | repomocks "github.com/bnema/dumber/internal/domain/repository/mocks" | |
| 13 | 13 | "github.com/stretchr/testify/assert" | |
| 14 | 14 | "github.com/stretchr/testify/mock" | |
| 15 | - | "github.com/stretchr/testify/require" | |
| 16 | 15 | ) | |
| 17 | 16 | ||
| 18 | 17 | func TestHandlePermissionUseCase_AutoAllowDisplayCapture(t *testing.T) { | |
| @@ -359,53 +358,26 @@ func TestHandlePermissionUseCase_NonPersistableNotSaved(t *testing.T) { | |||
| 359 | 358 | permRepo.AssertNotCalled(t, "Set") | |
| 360 | 359 | } | |
| 361 | 360 | ||
| 362 | - | func TestExtractOrigin_ValidURI(t *testing.T) { | |
| 363 | - | tests := []struct { | |
| 364 | - | name string | |
| 365 | - | uri string | |
| 366 | - | expected string | |
| 367 | - | }{ | |
| 368 | - | { | |
| 369 | - | name: "https with port", | |
| 370 | - | uri: "https://example.com:8443/path?query=1", | |
| 371 | - | expected: "https://example.com:8443", | |
| 372 | - | }, | |
| 373 | - | { | |
| 374 | - | name: "https without port", | |
| 375 | - | uri: "https://example.com/path", | |
| 376 | - | expected: "https://example.com", | |
| 377 | - | }, | |
| 378 | - | { | |
| 379 | - | name: "http", | |
| 380 | - | uri: "http://localhost:8080/app", | |
| 381 | - | expected: "http://localhost:8080", | |
| 382 | - | }, | |
| 383 | - | } | |
| 361 | + | func TestHandlePermissionUseCase_NoDialogPresenter(t *testing.T) { | |
| 362 | + | ctx := testContext() | |
| 363 | + | permRepo := repomocks.NewMockPermissionRepository(t) | |
| 384 | 364 | ||
| 385 | - | for _, tt := range tests { | |
| 386 | - | t.Run(tt.name, func(t *testing.T) { | |
| 387 | - | origin, err := usecase.ExtractOrigin(tt.uri) | |
| 388 | - | require.NoError(t, err) | |
| 389 | - | assert.Equal(t, tt.expected, origin) | |
| 390 | - | }) | |
| 391 | - | } | |
| 392 | - | } | |
| 365 | + | // Create use case with nil dialog presenter | |
| 366 | + | uc := usecase.NewHandlePermissionUseCase(permRepo, nil) | |
| 393 | 367 | ||
| 394 | - | func TestExtractOrigin_InvalidURI(t *testing.T) { | |
| 395 | - | tests := []struct { | |
| 396 | - | name string | |
| 397 | - | uri string | |
| 398 | - | }{ | |
| 399 | - | {name: "empty", uri: ""}, | |
| 400 | - | {name: "no scheme", uri: "example.com"}, | |
| 401 | - | {name: "no host", uri: "https://"}, | |
| 368 | + | denied := false | |
| 369 | + | callback := usecase.PermissionCallback{ | |
| 370 | + | Allow: func() {}, | |
| 371 | + | Deny: func() { denied = true }, | |
| 402 | 372 | } | |
| 403 | 373 | ||
| 404 | - | for _, tt := range tests { | |
| 405 | - | t.Run(tt.name, func(t *testing.T) { | |
| 406 | - | origin, err := usecase.ExtractOrigin(tt.uri) | |
| 407 | - | require.Error(t, err) | |
| 408 | - | assert.Empty(t, origin) | |
| 409 | - | }) | |
| 410 | - | } | |
| 374 | + | // Mic request with no stored permission and no dialog should be denied | |
| 375 | + | permRepo.EXPECT().Get(mock.Anything, "https://meet.example.com", entity.PermissionTypeMicrophone). | |
| 376 | + | Return(nil, nil) | |
| 377 | + | ||
| 378 | + | uc.HandlePermissionRequest(ctx, "https://meet.example.com", []entity.PermissionType{ | |
| 379 | + | entity.PermissionTypeMicrophone, | |
| 380 | + | }, callback) | |
| 381 | + | ||
| 382 | + | assert.True(t, denied, "should deny when no dialog presenter is available") | |
| 411 | 383 | } | |
| @@ -12,7 +12,6 @@ import ( | |||||
| 12 | repomocks "github.com/bnema/dumber/internal/domain/repository/mocks" | 12 | repomocks "github.com/bnema/dumber/internal/domain/repository/mocks" | ||
| 13 | "github.com/stretchr/testify/assert" | 13 | "github.com/stretchr/testify/assert" | ||
| 14 | "github.com/stretchr/testify/mock" | 14 | "github.com/stretchr/testify/mock" | ||
| 15 | - | "github.com/stretchr/testify/require" | |||
| 16 | ) | 15 | ) | ||
| 17 | 16 | ||||
| 18 | func TestHandlePermissionUseCase_AutoAllowDisplayCapture(t *testing.T) { | 17 | func TestHandlePermissionUseCase_AutoAllowDisplayCapture(t *testing.T) { | ||
| @@ -359,53 +358,26 @@ func TestHandlePermissionUseCase_NonPersistableNotSaved(t *testing.T) { | |||||
| 359 | permRepo.AssertNotCalled(t, "Set") | 358 | permRepo.AssertNotCalled(t, "Set") | ||
| 360 | } | 359 | } | ||
| 361 | 360 | ||||
| 362 | - | func TestExtractOrigin_ValidURI(t *testing.T) { | 361 | + | func TestHandlePermissionUseCase_NoDialogPresenter(t *testing.T) { |
| 363 | - | tests := []struct { | 362 | + | ctx := testContext() |
| 364 | - | name string | 363 | + | permRepo := repomocks.NewMockPermissionRepository(t) |
| 365 | - | uri string | |||
| 366 | - | expected string | |||
| 367 | - | }{ | |||
| 368 | - | { | |||
| 369 | - | name: "https with port", | |||
| 370 | - | uri: "https://example.com:8443/path?query=1", | |||
| 371 | - | expected: "https://example.com:8443", | |||
| 372 | - | }, | |||
| 373 | - | { | |||
| 374 | - | name: "https without port", | |||
| 375 | - | uri: "https://example.com/path", | |||
| 376 | - | expected: "https://example.com", | |||
| 377 | - | }, | |||
| 378 | - | { | |||
| 379 | - | name: "http", | |||
| 380 | - | uri: "http://localhost:8080/app", | |||
| 381 | - | expected: "http://localhost:8080", | |||
| 382 | - | }, | |||
| 383 | - | } | |||
| 384 | 364 | ||||
| 385 | - | for _, tt := range tests { | 365 | + | // Create use case with nil dialog presenter |
| 386 | - | t.Run(tt.name, func(t *testing.T) { | 366 | + | uc := usecase.NewHandlePermissionUseCase(permRepo, nil) |
| 387 | - | origin, err := usecase.ExtractOrigin(tt.uri) | |||
| 388 | - | require.NoError(t, err) | |||
| 389 | - | assert.Equal(t, tt.expected, origin) | |||
| 390 | - | }) | |||
| 391 | - | } | |||
| 392 | - | } | |||
| 393 | 367 | ||||
| 394 | - | func TestExtractOrigin_InvalidURI(t *testing.T) { | 368 | + | denied := false |
| 395 | - | tests := []struct { | 369 | + | callback := usecase.PermissionCallback{ |
| 396 | - | name string | 370 | + | Allow: func() {}, |
| 397 | - | uri string | 371 | + | Deny: func() { denied = true }, |
| 398 | - | }{ | |||
| 399 | - | {name: "empty", uri: ""}, | |||
| 400 | - | {name: "no scheme", uri: "example.com"}, | |||
| 401 | - | {name: "no host", uri: "https://"}, | |||
| 402 | } | 372 | } | ||
| 403 | 373 | ||||
| 404 | - | for _, tt := range tests { | 374 | + | // Mic request with no stored permission and no dialog should be denied |
| 405 | - | t.Run(tt.name, func(t *testing.T) { | 375 | + | permRepo.EXPECT().Get(mock.Anything, "https://meet.example.com", entity.PermissionTypeMicrophone). |
| 406 | - | origin, err := usecase.ExtractOrigin(tt.uri) | 376 | + | Return(nil, nil) |
| 407 | - | require.Error(t, err) | 377 | + | |
| 408 | - | assert.Empty(t, origin) | 378 | + | uc.HandlePermissionRequest(ctx, "https://meet.example.com", []entity.PermissionType{ |
| 409 | - | }) | 379 | + | entity.PermissionTypeMicrophone, |
| 410 | - | } | 380 | + | }, callback) |
| 381 | + | ||||
| 382 | + | assert.True(t, denied, "should deny when no dialog presenter is available") | |||
| 411 | } | 383 | } | ||
M internal/domain/entity/permission.go +0 -3
| @@ -14,9 +14,6 @@ const ( | |||
| 14 | 14 | // PermissionTypeCamera represents camera access permission. | |
| 15 | 15 | PermissionTypeCamera PermissionType = "camera" | |
| 16 | 16 | ||
| 17 | - | // PermissionTypeMicrophoneAndCamera represents combined mic and camera permission. | |
| 18 | - | PermissionTypeMicrophoneAndCamera PermissionType = "microphone_and_camera" | |
| 19 | - | ||
| 20 | 17 | // PermissionTypeDisplay represents screen sharing/display capture permission. | |
| 21 | 18 | PermissionTypeDisplay PermissionType = "display" | |
| 22 | 19 | ||
| @@ -14,9 +14,6 @@ const ( | |||||
| 14 | // PermissionTypeCamera represents camera access permission. | 14 | // PermissionTypeCamera represents camera access permission. | ||
| 15 | PermissionTypeCamera PermissionType = "camera" | 15 | PermissionTypeCamera PermissionType = "camera" | ||
| 16 | 16 | ||||
| 17 | - | // PermissionTypeMicrophoneAndCamera represents combined mic and camera permission. | |||
| 18 | - | PermissionTypeMicrophoneAndCamera PermissionType = "microphone_and_camera" | |||
| 19 | - | ||||
| 20 | // PermissionTypeDisplay represents screen sharing/display capture permission. | 17 | // PermissionTypeDisplay represents screen sharing/display capture permission. | ||
| 21 | PermissionTypeDisplay PermissionType = "display" | 18 | PermissionTypeDisplay PermissionType = "display" | ||
| 22 | 19 | ||||
M internal/domain/entity/permission_test.go +0 -2
| @@ -15,7 +15,6 @@ func TestPermissionType_Constants(t *testing.T) { | |||
| 15 | 15 | }{ | |
| 16 | 16 | {entity.PermissionTypeMicrophone, "microphone"}, | |
| 17 | 17 | {entity.PermissionTypeCamera, "camera"}, | |
| 18 | - | {entity.PermissionTypeMicrophoneAndCamera, "microphone_and_camera"}, | |
| 19 | 18 | {entity.PermissionTypeDisplay, "display"}, | |
| 20 | 19 | {entity.PermissionTypeDeviceInfo, "device_info"}, | |
| 21 | 20 | {entity.PermissionTypeClipboard, "clipboard"}, | |
| @@ -112,7 +111,6 @@ func TestCanPersist(t *testing.T) { | |||
| 112 | 111 | // Persistable | |
| 113 | 112 | {"microphone", entity.PermissionTypeMicrophone, true}, | |
| 114 | 113 | {"camera", entity.PermissionTypeCamera, true}, | |
| 115 | - | {"microphone and camera", entity.PermissionTypeMicrophoneAndCamera, true}, | |
| 116 | 114 | {"clipboard", entity.PermissionTypeClipboard, true}, | |
| 117 | 115 | {"notification", entity.PermissionTypeNotification, true}, | |
| 118 | 116 | {"geolocation", entity.PermissionTypeGeolocation, true}, | |
| @@ -15,7 +15,6 @@ func TestPermissionType_Constants(t *testing.T) { | |||||
| 15 | }{ | 15 | }{ | ||
| 16 | {entity.PermissionTypeMicrophone, "microphone"}, | 16 | {entity.PermissionTypeMicrophone, "microphone"}, | ||
| 17 | {entity.PermissionTypeCamera, "camera"}, | 17 | {entity.PermissionTypeCamera, "camera"}, | ||
| 18 | - | {entity.PermissionTypeMicrophoneAndCamera, "microphone_and_camera"}, | |||
| 19 | {entity.PermissionTypeDisplay, "display"}, | 18 | {entity.PermissionTypeDisplay, "display"}, | ||
| 20 | {entity.PermissionTypeDeviceInfo, "device_info"}, | 19 | {entity.PermissionTypeDeviceInfo, "device_info"}, | ||
| 21 | {entity.PermissionTypeClipboard, "clipboard"}, | 20 | {entity.PermissionTypeClipboard, "clipboard"}, | ||
| @@ -112,7 +111,6 @@ func TestCanPersist(t *testing.T) { | |||||
| 112 | // Persistable | 111 | // Persistable | ||
| 113 | {"microphone", entity.PermissionTypeMicrophone, true}, | 112 | {"microphone", entity.PermissionTypeMicrophone, true}, | ||
| 114 | {"camera", entity.PermissionTypeCamera, true}, | 113 | {"camera", entity.PermissionTypeCamera, true}, | ||
| 115 | - | {"microphone and camera", entity.PermissionTypeMicrophoneAndCamera, true}, | |||
| 116 | {"clipboard", entity.PermissionTypeClipboard, true}, | 114 | {"clipboard", entity.PermissionTypeClipboard, true}, | ||
| 117 | {"notification", entity.PermissionTypeNotification, true}, | 115 | {"notification", entity.PermissionTypeNotification, true}, | ||
| 118 | {"geolocation", entity.PermissionTypeGeolocation, true}, | 116 | {"geolocation", entity.PermissionTypeGeolocation, true}, | ||
M internal/domain/url/normalize.go +16 -0
| @@ -203,3 +203,19 @@ func IsExternalScheme(uri string) bool { | |||
| 203 | 203 | return true | |
| 204 | 204 | } | |
| 205 | 205 | } | |
| 206 | + | ||
| 207 | + | // ExtractOrigin extracts the origin (scheme://host) from a URI. | |
| 208 | + | // This normalizes URIs to origins for permission storage and comparison. | |
| 209 | + | // Example: "https://example.com/path?query" -> "https://example.com" | |
| 210 | + | func ExtractOrigin(uri string) (string, error) { | |
| 211 | + | parsed, err := url.Parse(uri) | |
| 212 | + | if err != nil { | |
| 213 | + | return "", fmt.Errorf("invalid URI: %w", err) | |
| 214 | + | } | |
| 215 | + | ||
| 216 | + | if parsed.Scheme == "" || parsed.Host == "" { | |
| 217 | + | return "", fmt.Errorf("URI missing scheme or host: %s", uri) | |
| 218 | + | } | |
| 219 | + | ||
| 220 | + | return parsed.Scheme + "://" + parsed.Host, nil | |
| 221 | + | } | |
| @@ -203,3 +203,19 @@ func IsExternalScheme(uri string) bool { | |||||
| 203 | return true | 203 | return true | ||
| 204 | } | 204 | } | ||
| 205 | } | 205 | } | ||
| 206 | + | ||||
| 207 | + | // ExtractOrigin extracts the origin (scheme://host) from a URI. | |||
| 208 | + | // This normalizes URIs to origins for permission storage and comparison. | |||
| 209 | + | // Example: "https://example.com/path?query" -> "https://example.com" | |||
| 210 | + | func ExtractOrigin(uri string) (string, error) { | |||
| 211 | + | parsed, err := url.Parse(uri) | |||
| 212 | + | if err != nil { | |||
| 213 | + | return "", fmt.Errorf("invalid URI: %w", err) | |||
| 214 | + | } | |||
| 215 | + | ||||
| 216 | + | if parsed.Scheme == "" || parsed.Host == "" { | |||
| 217 | + | return "", fmt.Errorf("URI missing scheme or host: %s", uri) | |||
| 218 | + | } | |||
| 219 | + | ||||
| 220 | + | return parsed.Scheme + "://" + parsed.Host, nil | |||
| 221 | + | } | |||
M internal/domain/url/normalize_test.go +57 -0
| @@ -509,3 +509,60 @@ func TestIsExternalScheme(t *testing.T) { | |||
| 509 | 509 | }) | |
| 510 | 510 | } | |
| 511 | 511 | } | |
| 512 | + | ||
| 513 | + | func TestExtractOrigin_ValidURI(t *testing.T) { | |
| 514 | + | tests := []struct { | |
| 515 | + | name string | |
| 516 | + | uri string | |
| 517 | + | expected string | |
| 518 | + | }{ | |
| 519 | + | { | |
| 520 | + | name: "https with port", | |
| 521 | + | uri: "https://example.com:8443/path?query=1", | |
| 522 | + | expected: "https://example.com:8443", | |
| 523 | + | }, | |
| 524 | + | { | |
| 525 | + | name: "https without port", | |
| 526 | + | uri: "https://example.com/path", | |
| 527 | + | expected: "https://example.com", | |
| 528 | + | }, | |
| 529 | + | { | |
| 530 | + | name: "http", | |
| 531 | + | uri: "http://localhost:8080/app", | |
| 532 | + | expected: "http://localhost:8080", | |
| 533 | + | }, | |
| 534 | + | } | |
| 535 | + | ||
| 536 | + | for _, tt := range tests { | |
| 537 | + | t.Run(tt.name, func(t *testing.T) { | |
| 538 | + | origin, err := ExtractOrigin(tt.uri) | |
| 539 | + | if err != nil { | |
| 540 | + | t.Errorf("ExtractOrigin(%q) returned error: %v", tt.uri, err) | |
| 541 | + | return | |
| 542 | + | } | |
| 543 | + | if origin != tt.expected { | |
| 544 | + | t.Errorf("ExtractOrigin(%q) = %q, want %q", tt.uri, origin, tt.expected) | |
| 545 | + | } | |
| 546 | + | }) | |
| 547 | + | } | |
| 548 | + | } | |
| 549 | + | ||
| 550 | + | func TestExtractOrigin_InvalidURI(t *testing.T) { | |
| 551 | + | tests := []struct { | |
| 552 | + | name string | |
| 553 | + | uri string | |
| 554 | + | }{ | |
| 555 | + | {name: "empty", uri: ""}, | |
| 556 | + | {name: "no scheme", uri: "example.com"}, | |
| 557 | + | {name: "no host", uri: "https://"}, | |
| 558 | + | } | |
| 559 | + | ||
| 560 | + | for _, tt := range tests { | |
| 561 | + | t.Run(tt.name, func(t *testing.T) { | |
| 562 | + | origin, err := ExtractOrigin(tt.uri) | |
| 563 | + | if err == nil { | |
| 564 | + | t.Errorf("ExtractOrigin(%q) should return error, got origin: %q", tt.uri, origin) | |
| 565 | + | } | |
| 566 | + | }) | |
| 567 | + | } | |
| 568 | + | } | |
| @@ -509,3 +509,60 @@ func TestIsExternalScheme(t *testing.T) { | |||||
| 509 | }) | 509 | }) | ||
| 510 | } | 510 | } | ||
| 511 | } | 511 | } | ||
| 512 | + | ||||
| 513 | + | func TestExtractOrigin_ValidURI(t *testing.T) { | |||
| 514 | + | tests := []struct { | |||
| 515 | + | name string | |||
| 516 | + | uri string | |||
| 517 | + | expected string | |||
| 518 | + | }{ | |||
| 519 | + | { | |||
| 520 | + | name: "https with port", | |||
| 521 | + | uri: "https://example.com:8443/path?query=1", | |||
| 522 | + | expected: "https://example.com:8443", | |||
| 523 | + | }, | |||
| 524 | + | { | |||
| 525 | + | name: "https without port", | |||
| 526 | + | uri: "https://example.com/path", | |||
| 527 | + | expected: "https://example.com", | |||
| 528 | + | }, | |||
| 529 | + | { | |||
| 530 | + | name: "http", | |||
| 531 | + | uri: "http://localhost:8080/app", | |||
| 532 | + | expected: "http://localhost:8080", | |||
| 533 | + | }, | |||
| 534 | + | } | |||
| 535 | + | ||||
| 536 | + | for _, tt := range tests { | |||
| 537 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 538 | + | origin, err := ExtractOrigin(tt.uri) | |||
| 539 | + | if err != nil { | |||
| 540 | + | t.Errorf("ExtractOrigin(%q) returned error: %v", tt.uri, err) | |||
| 541 | + | return | |||
| 542 | + | } | |||
| 543 | + | if origin != tt.expected { | |||
| 544 | + | t.Errorf("ExtractOrigin(%q) = %q, want %q", tt.uri, origin, tt.expected) | |||
| 545 | + | } | |||
| 546 | + | }) | |||
| 547 | + | } | |||
| 548 | + | } | |||
| 549 | + | ||||
| 550 | + | func TestExtractOrigin_InvalidURI(t *testing.T) { | |||
| 551 | + | tests := []struct { | |||
| 552 | + | name string | |||
| 553 | + | uri string | |||
| 554 | + | }{ | |||
| 555 | + | {name: "empty", uri: ""}, | |||
| 556 | + | {name: "no scheme", uri: "example.com"}, | |||
| 557 | + | {name: "no host", uri: "https://"}, | |||
| 558 | + | } | |||
| 559 | + | ||||
| 560 | + | for _, tt := range tests { | |||
| 561 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 562 | + | origin, err := ExtractOrigin(tt.uri) | |||
| 563 | + | if err == nil { | |||
| 564 | + | t.Errorf("ExtractOrigin(%q) should return error, got origin: %q", tt.uri, origin) | |||
| 565 | + | } | |||
| 566 | + | }) | |||
| 567 | + | } | |||
| 568 | + | } | |||
M internal/infrastructure/webkit/webview.go +1 -15
| @@ -950,7 +950,7 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||
| 950 | 950 | wv.logger.Debug().Msg("permission request with empty origin, denying") | |
| 951 | 951 | return false | |
| 952 | 952 | } | |
| 953 | - | origin, err := extractOrigin(uri) | |
| 953 | + | origin, err := urlutil.ExtractOrigin(uri) | |
| 954 | 954 | if err != nil { | |
| 955 | 955 | wv.logger.Debug().Str("uri", uri).Err(err).Msg("permission request: failed to extract origin, denying") | |
| 956 | 956 | return false | |
| @@ -978,7 +978,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||
| 978 | 978 | ||
| 979 | 979 | allow := func() { | |
| 980 | 980 | if allowCalled || denyCalled { | |
| 981 | - | requestObj.Unref() | |
| 982 | 981 | return | |
| 983 | 982 | } | |
| 984 | 983 | allowCalled = true | |
| @@ -988,7 +987,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||
| 988 | 987 | ||
| 989 | 988 | deny := func() { | |
| 990 | 989 | if allowCalled || denyCalled { | |
| 991 | - | requestObj.Unref() | |
| 992 | 990 | return | |
| 993 | 991 | } | |
| 994 | 992 | denyCalled = true | |
| @@ -1004,18 +1002,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||
| 1004 | 1002 | wv.signalIDs = append(wv.signalIDs, uintptr(sigID)) | |
| 1005 | 1003 | } | |
| 1006 | 1004 | ||
| 1007 | - | // extractOrigin extracts the origin (scheme://host) from a URI. | |
| 1008 | - | func extractOrigin(uri string) (string, error) { | |
| 1009 | - | u, err := url.Parse(uri) | |
| 1010 | - | if err != nil { | |
| 1011 | - | return "", err | |
| 1012 | - | } | |
| 1013 | - | if u.Scheme == "" || u.Host == "" { | |
| 1014 | - | return "", fmt.Errorf("URI missing scheme or host") | |
| 1015 | - | } | |
| 1016 | - | return u.Scheme + "://" + u.Host, nil | |
| 1017 | - | } | |
| 1018 | - | ||
| 1019 | 1005 | // determinePermissionTypes extracts permission types from a WebKit permission request. | |
| 1020 | 1006 | // This uses type checking to identify UserMediaPermissionRequest and its specific types. | |
| 1021 | 1007 | func (wv *WebView) determinePermissionTypes(requestPtr uintptr) []string { | |
| @@ -950,7 +950,7 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||||
| 950 | wv.logger.Debug().Msg("permission request with empty origin, denying") | 950 | wv.logger.Debug().Msg("permission request with empty origin, denying") | ||
| 951 | return false | 951 | return false | ||
| 952 | } | 952 | } | ||
| 953 | - | origin, err := extractOrigin(uri) | 953 | + | origin, err := urlutil.ExtractOrigin(uri) |
| 954 | if err != nil { | 954 | if err != nil { | ||
| 955 | wv.logger.Debug().Str("uri", uri).Err(err).Msg("permission request: failed to extract origin, denying") | 955 | wv.logger.Debug().Str("uri", uri).Err(err).Msg("permission request: failed to extract origin, denying") | ||
| 956 | return false | 956 | return false | ||
| @@ -978,7 +978,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||||
| 978 | 978 | ||||
| 979 | allow := func() { | 979 | allow := func() { | ||
| 980 | if allowCalled || denyCalled { | 980 | if allowCalled || denyCalled { | ||
| 981 | - | requestObj.Unref() | |||
| 982 | return | 981 | return | ||
| 983 | } | 982 | } | ||
| 984 | allowCalled = true | 983 | allowCalled = true | ||
| @@ -988,7 +987,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||||
| 988 | 987 | ||||
| 989 | deny := func() { | 988 | deny := func() { | ||
| 990 | if allowCalled || denyCalled { | 989 | if allowCalled || denyCalled { | ||
| 991 | - | requestObj.Unref() | |||
| 992 | return | 990 | return | ||
| 993 | } | 991 | } | ||
| 994 | denyCalled = true | 992 | denyCalled = true | ||
| @@ -1004,18 +1002,6 @@ func (wv *WebView) connectPermissionRequestSignal() { | |||||
| 1004 | wv.signalIDs = append(wv.signalIDs, uintptr(sigID)) | 1002 | wv.signalIDs = append(wv.signalIDs, uintptr(sigID)) | ||
| 1005 | } | 1003 | } | ||
| 1006 | 1004 | ||||
| 1007 | - | // extractOrigin extracts the origin (scheme://host) from a URI. | |||
| 1008 | - | func extractOrigin(uri string) (string, error) { | |||
| 1009 | - | u, err := url.Parse(uri) | |||
| 1010 | - | if err != nil { | |||
| 1011 | - | return "", err | |||
| 1012 | - | } | |||
| 1013 | - | if u.Scheme == "" || u.Host == "" { | |||
| 1014 | - | return "", fmt.Errorf("URI missing scheme or host") | |||
| 1015 | - | } | |||
| 1016 | - | return u.Scheme + "://" + u.Host, nil | |||
| 1017 | - | } | |||
| 1018 | - | ||||
| 1019 | // determinePermissionTypes extracts permission types from a WebKit permission request. | 1005 | // determinePermissionTypes extracts permission types from a WebKit permission request. | ||
| 1020 | // This uses type checking to identify UserMediaPermissionRequest and its specific types. | 1006 | // This uses type checking to identify UserMediaPermissionRequest and its specific types. | ||
| 1021 | func (wv *WebView) determinePermissionTypes(requestPtr uintptr) []string { | 1007 | func (wv *WebView) determinePermissionTypes(requestPtr uintptr) []string { | ||
M internal/ui/dialog/permission.go +2 -2
| @@ -104,7 +104,7 @@ func (d *PermissionDialog) buildHeading(permTypes []entity.PermissionType) strin | |||
| 104 | 104 | ||
| 105 | 105 | for _, pt := range permTypes { | |
| 106 | 106 | switch pt { | |
| 107 | - | case entity.PermissionTypeMicrophone, entity.PermissionTypeMicrophoneAndCamera: | |
| 107 | + | case entity.PermissionTypeMicrophone: | |
| 108 | 108 | hasMic = true | |
| 109 | 109 | case entity.PermissionTypeCamera: | |
| 110 | 110 | hasCam = true | |
| @@ -130,7 +130,7 @@ func (d *PermissionDialog) buildBody(origin string, permTypes []entity.Permissio | |||
| 130 | 130 | ||
| 131 | 131 | for _, pt := range permTypes { | |
| 132 | 132 | switch pt { | |
| 133 | - | case entity.PermissionTypeMicrophone, entity.PermissionTypeMicrophoneAndCamera: | |
| 133 | + | case entity.PermissionTypeMicrophone: | |
| 134 | 134 | hasMic = true | |
| 135 | 135 | case entity.PermissionTypeCamera: | |
| 136 | 136 | hasCam = true | |
| @@ -104,7 +104,7 @@ func (d *PermissionDialog) buildHeading(permTypes []entity.PermissionType) strin | |||||
| 104 | 104 | ||||
| 105 | for _, pt := range permTypes { | 105 | for _, pt := range permTypes { | ||
| 106 | switch pt { | 106 | switch pt { | ||
| 107 | - | case entity.PermissionTypeMicrophone, entity.PermissionTypeMicrophoneAndCamera: | 107 | + | case entity.PermissionTypeMicrophone: |
| 108 | hasMic = true | 108 | hasMic = true | ||
| 109 | case entity.PermissionTypeCamera: | 109 | case entity.PermissionTypeCamera: | ||
| 110 | hasCam = true | 110 | hasCam = true | ||
| @@ -130,7 +130,7 @@ func (d *PermissionDialog) buildBody(origin string, permTypes []entity.Permissio | |||||
| 130 | 130 | ||||
| 131 | for _, pt := range permTypes { | 131 | for _, pt := range permTypes { | ||
| 132 | switch pt { | 132 | switch pt { | ||
| 133 | - | case entity.PermissionTypeMicrophone, entity.PermissionTypeMicrophoneAndCamera: | 133 | + | case entity.PermissionTypeMicrophone: |
| 134 | hasMic = true | 134 | hasMic = true | ||
| 135 | case entity.PermissionTypeCamera: | 135 | case entity.PermissionTypeCamera: | ||
| 136 | hasCam = true | 136 | hasCam = true | ||