Removed PhotosAddOnly permission request within MediaPicker.ios#34287
Removed PhotosAddOnly permission request within MediaPicker.ios#34287kubaflo merged 1 commit intodotnet:inflight/currentfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34287Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34287" |
|
Hey there @@Kyranio! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
While that might be true, doesn't our implementation always save it to the gallery on iOS? We don't have an option to set the right now (see #30439), so because of that, we also need to have this permission now. If we make that optional for .NET 11, then we can make this permission optional as well |
If that is the case, then this might actually be a bug instead, as a photo taken with iOS (handled with MediaPicker) does in fact not save it to the gallery. The source code of the MediaPicker PhotoAsync method itself does not contain any logic indicating that the image is to be saved to the device itself, nor to it's gallery... which was why I initially posted the issue #34246 (under a different account, related to the company I work for) and created this pullrequest. It's also why I referenced the official documentation which contains a code-snippet, suggesting that a developer needs to save the image to the device after calling CapturePhotoAsync themselves, in their own code. This in itself led me to believe that requesting the permission shouldn't be within the PhotoAsync method but rather within the user's own code. If the PhotoAsync task should save the picture to the device's gallery regardless, the documentation would probably need to be updated. The PhotoAsync method might need to be looked at, however, as in our experience it does not add the pictures to the gallery by itself... |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34287 | Remove the PhotosAddOnly permission request before UIImagePickerController camera flow |
❌ Gate FAILED | src/Essentials/src/MediaPicker/MediaPicker.ios.cs |
Original PR; merged |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Add SaveToGallery opt-in to MediaPickerOptions; condition PhotosAddOnly request on it |
2 files + PublicAPI entries | Adds public API; simpler removal is better since MAUI never saves to gallery | |
| 2 | try-fix | Check NSPhotoLibraryAddUsageDescription in Info.plist at runtime; skip permission if key absent |
1 file | Technically sound but adds complexity for a path that shouldn't need the permission at all | |
| 3 | try-fix | AppContext switch Microsoft.Maui.MediaPicker.RequestPhotoAddPermission to gate legacy behavior |
1 file | Adds maintenance burden for a regression fix that needs no opt-out | |
| 4 | try-fix | Remove permission + add explanatory comment documenting why | 1 file | Close to PR but with documentation value; marginally better but comment not strictly necessary | |
| PR | PR #34287 | Remove PhotosAddOnly permission request from iOS capture path |
❌ Gate FAILED | 1 file | Original PR — merged; simplest and correct |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | All approaches are variations on unconditional vs conditional removal; PR's unconditional removal is correct |
| gpt-5.3-codex | 2 | Yes (flawed) | Suggested fail-open + fallback on photo-library write error — assessed as technically flawed since UIImagePickerController.Camera never writes to gallery, so no error to catch |
| claude-sonnet-4.6 | 2 | No | PR's removal is provably correct; all alternatives add unnecessary complexity |
Exhausted: Yes
Selected Fix: PR #34287 — The simple 5-line removal is the most correct and minimal fix. All 3 alternative approaches (SaveToGallery property, plist check, AppContext switch) add complexity around a permission that guards no actual operation. The gpt-5.3-codex cross-pollination idea was technically flawed. The fix was merged after extensive maintainer code review confirming UIImagePickerController.Camera never calls PHPhotoLibrary or any gallery-write API.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | 1 iOS implementation file changed; 0 tests added |
| Gate | ❌ FAILED | Tests did not behave as expected — no validating tests exist in the PR |
| Try-Fix | ✅ COMPLETE | 4 attempts (all Blocked — iOS unavailable on Android agent); PR's fix selected as best |
| Report | ✅ COMPLETE |
Summary
PR #34287 removes an unnecessary Permissions.PhotosAddOnly request from the iOS UIImagePickerController camera capture path in MediaPicker.ios.cs. The change is logically correct and was confirmed by two independent human reviews and three AI analyses before merge. The gate failed because the PR includes no automated tests and the requested review platform (Android) cannot exercise this iOS-only behavior. All 4 alternative fix approaches explored via try-fix were either more complex than needed (adding SaveToGallery API, plist check, AppContext switch) or equivalent to the PR's fix (removal + comment). The PR's simple removal is the best implementation — but the absence of test coverage is a real gap.
Root Cause
The PhotosAddOnly permission request was erroneously introduced during the MediaPicker iOS modernization (PR #28920). The UIImagePickerController.Camera delegate in MAUI extracts captured media via DictionaryToMediaFile and returns a FileResult — it never calls UIImageWriteToSavedPhotosAlbum, PHPhotoLibrary.PerformChanges, or any photo-library write API. The permission was guarding a non-existent gallery-save operation, requiring apps to declare NSPhotoLibraryAddUsageDescription in their Info.plist even when they only needed camera access.
Fix Quality
The PR's fix (5-line removal) is the strongest of all explored candidates:
- Correct: Confirmed by code tracing — no gallery-write call exists in the capture path
- Minimal: Single file, 5 lines removed, no new API surface
- Merged: Maintainer kubaflo confirmed the fix after independent code review
- Gap: No automated test coverage validating the permission behavior before/after the change — the only testing is manual verification on iOS
MauiBot
added
s/agent-changes-requested
Independent Code ReviewI traced the full iOS capture flow end-to-end to verify whether this PR is correct or should be closed. Finding: The PR is valid and should be merged (not closed)The
Addressing @jfversluis's concern
the code does not save to the gallery. Issue #30439 ("Implement RecommendationThis PR correctly removes an unnecessary permission request that forces apps to declare The concern about test coverage from the bot review is understandable, but this is a permission-check the behavior of the capture flow itself is unchanged. The risk is minimal.removal |
|
We ran this PR through three independent AI models for a second opinion. Each was given the full code context, the maintainer concern, and the issue history. Verdict Merge:
Verdict Merge:
Verdict Merge:
Unanimous agreement across all three models: The |
### Description of Change Removed permission request for adding photos to the devices library on iOS, inside MediaPicker.ios, which doesn't need this permission at this spot inside the code as it does not add the newly captured photo to the device's library here. Given the [official documentation](https://learn.microsoft.com/en-us/dotnet/maui/platform-integration/device-media/picker?view=net-maui-10.0&tabs=windows#take-a-photo), adding the picture to the device's library is an action given in the example _after_ the CapturePhotoAsync is called, if the task's result contains a value. This action is completely optional, the developer could implement different logic for handling the picture data after calling CapturePhotoAsync, meaning the permission request becomes unnecessary and redundant. It is up to the developer to save the image to the device, which then requires the permission, meaning the developer should implement the permission request within their own logic. ### Issues Fixed Fixes #34246
🚦 Gate — Test Verification |
…et#34287) ### Description of Change Removed permission request for adding photos to the devices library on iOS, inside MediaPicker.ios, which doesn't need this permission at this spot inside the code as it does not add the newly captured photo to the device's library here. Given the [official documentation](https://learn.microsoft.com/en-us/dotnet/maui/platform-integration/device-media/picker?view=net-maui-10.0&tabs=windows#take-a-photo), adding the picture to the device's library is an action given in the example _after_ the CapturePhotoAsync is called, if the task's result contains a value. This action is completely optional, the developer could implement different logic for handling the picture data after calling CapturePhotoAsync, meaning the permission request becomes unnecessary and redundant. It is up to the developer to save the image to the device, which then requires the permission, meaning the developer should implement the permission request within their own logic. ### Issues Fixed Fixes dotnet#34246
### Description of Change Removed permission request for adding photos to the devices library on iOS, inside MediaPicker.ios, which doesn't need this permission at this spot inside the code as it does not add the newly captured photo to the device's library here. Given the [official documentation](https://learn.microsoft.com/en-us/dotnet/maui/platform-integration/device-media/picker?view=net-maui-10.0&tabs=windows#take-a-photo), adding the picture to the device's library is an action given in the example _after_ the CapturePhotoAsync is called, if the task's result contains a value. This action is completely optional, the developer could implement different logic for handling the picture data after calling CapturePhotoAsync, meaning the permission request becomes unnecessary and redundant. It is up to the developer to save the image to the device, which then requires the permission, meaning the developer should implement the permission request within their own logic. ### Issues Fixed Fixes #34246
4 participants
Description of Change
Removed permission request for adding photos to the devices library on iOS, inside MediaPicker.ios, which doesn't need this permission at this spot inside the code as it does not add the newly captured photo to the device's library here.
Given the official documentation, adding the picture to the device's library is an action given in the example after the CapturePhotoAsync is called, if the task's result contains a value.
This action is completely optional, the developer could implement different logic for handling the picture data after calling CapturePhotoAsync, meaning the permission request becomes unnecessary and redundant.
It is up to the developer to save the image to the device, which then requires the permission, meaning the developer should implement the permission request within their own logic.
Issues Fixed
Fixes #34246
Fixes #34661