Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[android][media-library] With media management permission, addAssetsToAlbumAsync fails on albums containing folders #30869

Closed
Nafeij opened this issue Aug 7, 2024 · 0 comments · Fixed by #31027
Labels
needs review Issue is ready to be reviewed by a maintainer

Comments

@Nafeij
Copy link
Contributor

Nafeij commented Aug 7, 2024

Minimal reproducible example

https://github.com/Nafeij/my-app

What platform(s) does this occur on?

Android

Where did you reproduce the issue?

in a development build

Summary

When adding assets to an album, the above check is performed with the assumption that this is a migrated or app-created album. However, it is still possible for apps to obtain MANAGE_MEDIA permissions and operate on external albums, which may themselves contain nested albums or folders. In this case, an unexpected error is thrown stating that the Media Library is corrupted.

To reproduce:

Prepare an album containing an image and a folder and run the reproducible example to add another image to said album.

Environment

expo-env-info 1.2.0 environment info:
    System:
      OS: Windows 11 10.0.22631
    Binaries:
      Node: 20.13.1 - C:\Program Files\nodejs\node.EXE
      Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD
      npm: 10.5.2 - C:\Program Files\nodejs\npm.CMD
    IDEs:
      Android Studio: AI-241.15989.150.2411.11948838
    npmPackages:
      expo: ~51.0.24 => 51.0.24 
      expo-router: ~3.5.20 => 3.5.20 
      react: 18.2.0 => 18.2.0 
      react-dom: 18.2.0 => 18.2.0 
      react-native: 0.74.3 => 0.74.3 
      react-native-web: ~0.19.10 => 0.19.12 
    Expo Workflow: bare

Expo Doctor Diagnostics

✔ Check Expo config for common issues
✔ Check package.json for common issues
✔ Check native tooling versions
✔ Check dependencies for packages that should not be installed directly
✔ Check for common project setup issues
✔ Check for issues with metro config
✔ Check npm/ yarn versions
✔ Check Expo config (app.json/ app.config.js) schema
✔ Check for legacy global CLI installed locally
✔ Check that packages match versions required by installed Expo SDK
✔ Check that native modules do not use incompatible support packages
✔ Check that native modules use compatible support package versions for installed Expo SDK

Didn't find any issues with the project!
@Nafeij Nafeij added the needs validation Issue needs to be validated label Aug 7, 2024
@expo-bot expo-bot added needs review Issue is ready to be reviewed by a maintainer and removed needs validation Issue needs to be validated labels Aug 7, 2024
@Nafeij Nafeij changed the title [expo-media-library] With media management permission, addAssetsToAlbumAsync fails on albums containing folders [android][media-library] With media management permission, addAssetsToAlbumAsync fails on albums containing folders Aug 16, 2024
behenate pushed a commit that referenced this issue Aug 30, 2024
# Why

We can't assume an album is corrupted because it contains non-files. In
an app with file manager perms, it's possible for expo to access
external albums which may be nested or otherwise contain directories.

Closes #30869

~~Also feel free to suggest alternative checks.~~ We should also check
if the non-file is a non-directory
(#31027 (comment))

# How

<!--
How did you build this feature or fix this bug and why?
-->

Add an additional check to prevent the exception being thrown for nested
but otherwise valid albums.

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

See the `Minimal reproducible example` in #30869 to reproduce original
behavior.

Before:


https://github.com/user-attachments/assets/f6edbc3f-2850-4ae6-97fd-12caa8ce4ae9

After:


https://github.com/user-attachments/assets/8527535f-5926-488f-82fd-bd6ee82956aa

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [x] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [x] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
behenate pushed a commit that referenced this issue Sep 30, 2024
# Why

We can't assume an album is corrupted because it contains non-files. In
an app with file manager perms, it's possible for expo to access
external albums which may be nested or otherwise contain directories.

Closes #30869

~~Also feel free to suggest alternative checks.~~ We should also check
if the non-file is a non-directory
(#31027 (comment))

# How

<!--
How did you build this feature or fix this bug and why?
-->

Add an additional check to prevent the exception being thrown for nested
but otherwise valid albums.

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

See the `Minimal reproducible example` in #30869 to reproduce original
behavior.

Before:


https://github.com/user-attachments/assets/f6edbc3f-2850-4ae6-97fd-12caa8ce4ae9

After:


https://github.com/user-attachments/assets/8527535f-5926-488f-82fd-bd6ee82956aa

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [x] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [x] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Issue is ready to be reviewed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants