Remove-External-File-Directory Permission #554
Conversation
prateek-kommunicate
changed the title
|
@prateek-kommunicate Please add description in PR |
|
@adarshmishra added;/ |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve significant modifications to two Java classes: Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
kommunicate/src/main/java/com/applozic/mobicommons/commons/image/ImageUtils.java (1)
162-178: Security: Consider implications of removing internal storage usage.The commented-out implementation stored images in internal storage, which provides app-private storage. Removing this without a proper alternative might lead developers to use less secure storage options.
Consider:
- Document the recommended secure alternative for image storage
- If external storage is the concern, explicitly use
context.getFilesDir()for app-private storage- Add migration guide for existing users
Would you like me to:
- Generate a secure alternative implementation using modern Android storage best practices?
- Create documentation for proper image storage patterns?
kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/FileClientService.java (3)
Line range hint
165-213: Update loadContactsvCard to handle null file path.The
loadContactsvCardmethod needs to be updated to handle the null return value fromgetFilePath.public void loadContactsvCard(Message message) { File file = null; HttpURLConnection connection = null; try { InputStream inputStream = null; FileMeta fileMeta = message.getFileMetas(); String contentType = fileMeta.getContentType(); String fileName = fileMeta.getName(); file = FileClientService.getFilePath(fileName, context.getApplicationContext(), contentType); + if (file == null) { + Utils.printLog(context, TAG, "File path could not be created: external storage access is deprecated"); + return; + } if (!file.exists()) {
Line range hint
108-164: Update loadThumbnailImage to handle null file path.The
loadThumbnailImagemethod also needs to be updated to handle the null return value fromgetFilePath.public Bitmap loadThumbnailImage(Context context, Message message, int reqWidth, int reqHeight) { HttpURLConnection connection = null; try { Bitmap attachedImage = null; String thumbnailUrl = new URLServiceProvider(context).getThumbnailURL(message); if (TextUtils.isEmpty(thumbnailUrl)) { return null; } String contentType = message.getFileMetas().getContentType(); final BitmapFactory.Options options = new BitmapFactory.Options(); options.inJustDecodeBounds = true; String imageName = FileUtils.getName(message.getFileMetas().getName()) + message.getCreatedAtTime() + "." + FileUtils.getFileFormat(message.getFileMetas().getName()); String imageLocalPath = getFilePath(imageName, context, message.getFileMetas().getContentType(), true).getAbsolutePath(); + if (imageLocalPath == null) { + Utils.printLog(context, TAG, "Image path could not be created: external storage access is deprecated"); + return null; + }
71-101: Consider implementing alternative storage solution.Since external storage access is being removed, consider implementing an alternative storage solution using app-specific storage or content providers.
Some alternatives to consider:
- Use
context.getFilesDir()for internal storage- Use
MediaStoreAPI for media files- Use
FileProviderfor sharing files with other apps- Use
context.getCacheDir()for temporary filesWould you like me to help implement any of these alternatives?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/FileClientService.java(2 hunks)kommunicate/src/main/java/com/applozic/mobicommons/commons/image/ImageUtils.java(1 hunks)
🔇 Additional comments (1)
kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/FileClientService.java (1)
44-62: Verify the removal of path constants across the codebase.
The removal of these path-related constants aligns with the goal of removing external directory permissions. However, we should verify that these constants are not referenced elsewhere in the codebase.
✅ Verification successful
Remove commented-out constants and their usages
The search results show that these constants are only referenced within the same file FileClientService.java in commented-out code blocks. Additionally, there's one active usage of text_card constant in MobiComConversationFragment.java, but it's defined locally in that file and not dependent on the one being removed.
Let's verify if the commented-out code blocks are the only remaining references:
Safe to remove the commented-out constants
The commented-out constants in FileClientService.java can be safely removed as:
- All references to these constants within
FileClientService.javaare already commented out - The path literals found in other files (
/files/v2/upload,/files/get/,/files/url?key=,/files/upload) are unrelated API endpoints and not related to the local storage paths being removed - The
text_cardconstant inMobiComConversationFragment.javais independently defined and not referencing the one being removed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed constants
echo "Searching for references to removed constants..."
rg "MOBI_COM_IMAGES_FOLDER|MOBI_COM_VIDEOS_FOLDER|MOBI_COM_CONTACT_FOLDER|MOBI_COM_OTHER_FILES_FOLDER|MOBI_COM_THUMBNAIL_SUFIX|IMAGE_DIR|THUMBNAIL_URL|MAIN_FOLDER_META_DATA|text_card" --type java
Length of output: 3343
Script:
#!/bin/bash
# Check the implementation of getFilePath and any other methods that might use these constants
ast-grep --pattern 'class FileClientService {
$$$
getFilePath($$$) {
$$$
}
$$$
}'
# Also check for any non-commented references to these paths as string literals
rg -l "/image|/video|/contact|/other|/.Thumbnail|/files/" --type java
Length of output: 1112
Script:
#!/bin/bash
# Check the actual usage of these paths in the identified files
echo "Checking MessageClientService.java..."
rg -A 2 "/image|/video|/contact|/other|/.Thumbnail|/files/" kommunicate/src/main/java/com/applozic/mobicomkit/api/conversation/MessageClientService.java
echo -e "\nChecking AttachmentManager.java..."
rg -A 2 "/image|/video|/contact|/other|/.Thumbnail|/files/" kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/AttachmentManager.java
echo -e "\nChecking ApplozicMongoStorageService.java..."
rg -A 2 "/image|/video|/contact|/other|/.Thumbnail|/files/" kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/urlservice/ApplozicMongoStorageService.java
echo -e "\nChecking GoogleCloudURLService.java..."
rg -A 2 "/image|/video|/contact|/other|/.Thumbnail|/files/" kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/urlservice/GoogleCloudURLService.java
echo -e "\nChecking KmAttachmentsController.java..."
rg -A 2 "/image|/video|/contact|/other|/.Thumbnail|/files/" kommunicateui/src/main/java/com/applozic/mobicomkit/uiwidgets/kommunicate/KmAttachmentsController.java
Length of output: 2686
| // FileOutputStream fos = null; | ||
| // try { | ||
| // fos = new FileOutputStream(file); | ||
| // // Use the compress method on the BitMap object to write image to the OutputStream | ||
| // bitmapImage.compress(Bitmap.CompressFormat.PNG, 100, fos); | ||
| // } catch (Exception e) { | ||
| // e.printStackTrace(); | ||
| // } finally { | ||
| // if (fos != null) { | ||
| // try { | ||
| // fos.close(); | ||
| // } catch (IOException e) { | ||
| // e.printStackTrace(); | ||
| // } | ||
| // } | ||
| // } | ||
| return ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging.
Simply returning an empty string silently fails without any indication of what went wrong.
Consider adding proper error handling:
public static String saveImageToInternalStorage(File file, Bitmap bitmapImage) {
- return "";
+ Log.w(TAG, "Image storage operation skipped - functionality deprecated");
+ throw new UnsupportedOperationException(
+ "Image storage to external directory is no longer supported. " +
+ "Please use MediaStore API for Android 10+ or context.getFilesDir() for internal storage."
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FileOutputStream fos = null; | |
| // try { | |
| // fos = new FileOutputStream(file); | |
| // // Use the compress method on the BitMap object to write image to the OutputStream | |
| // bitmapImage.compress(Bitmap.CompressFormat.PNG, 100, fos); | |
| // } catch (Exception e) { | |
| // e.printStackTrace(); | |
| // } finally { | |
| // if (fos != null) { | |
| // try { | |
| // fos.close(); | |
| // } catch (IOException e) { | |
| // e.printStackTrace(); | |
| // } | |
| // } | |
| // } | |
| return ""; | |
| // FileOutputStream fos = null; | |
| // try { | |
| // fos = new FileOutputStream(file); | |
| // // Use the compress method on the BitMap object to write image to the OutputStream | |
| // bitmapImage.compress(Bitmap.CompressFormat.PNG, 100, fos); | |
| // } catch (Exception e) { | |
| // e.printStackTrace(); | |
| // } finally { | |
| // if (fos != null) { | |
| // try { | |
| // fos.close(); | |
| // } catch (IOException e) { | |
| // e.printStackTrace(); | |
| // } | |
| // } | |
| // } | |
| Log.w(TAG, "Image storage operation skipped - functionality deprecated"); | |
| throw new UnsupportedOperationException( | |
| "Image storage to external directory is no longer supported. " + | |
| "Please use MediaStore API for Android 10+ or context.getFilesDir() for internal storage." | |
| ); |
💡 Codebase verification
Returning empty string breaks image storage functionality across multiple components
The verification reveals that returning an empty string from saveImageToInternalStorage is problematic because:
-
The method is used in critical image storage flows:
- Contact profile images in
AppContactService - File attachments in
FileClientService - Topic images in conversation flows
- Contact profile images in
-
While callers do check for empty strings using
TextUtils.isEmpty(), these checks are designed for optional image scenarios, not for handling storage failures. The empty string return effectively turns all image saves into failures. -
The original implementation stored images securely in internal storage. Removing this without providing an alternative storage mechanism breaks the app's image handling capabilities.
Recommendation:
public static String saveImageToInternalStorage(File file, Bitmap bitmapImage) {
if (file == null || bitmapImage == null) {
return null;
}
try (FileOutputStream fos = new FileOutputStream(file)) {
bitmapImage.compress(Bitmap.CompressFormat.PNG, 100, fos);
return file.getAbsolutePath();
} catch (IOException e) {
Log.e(TAG, "Failed to save image", e);
return null;
}
}🔗 Analysis chain
Critical: Method contract violation introduces potential runtime errors.
The changes to saveImageToInternalStorage introduce several concerns:
- The method signature suggests it should return a file path, but now returns an empty string
- Parameters
fileandbitmapImageare now unused - Callers expecting a valid file path will likely fail
Consider these alternatives:
public static String saveImageToInternalStorage(File file, Bitmap bitmapImage) {
- return "";
+ if (file == null || bitmapImage == null) {
+ return null;
+ }
+ // If external storage is deprecated, use app-specific internal storage
+ File internalDir = context.getFilesDir();
+ File imageFile = new File(internalDir, file.getName());
+
+ try (FileOutputStream fos = new FileOutputStream(imageFile)) {
+ bitmapImage.compress(Bitmap.CompressFormat.PNG, 100, fos);
+ return imageFile.getAbsolutePath();
+ } catch (IOException e) {
+ Log.e(TAG, "Failed to save image", e);
+ return null;
+ }
}Let's verify the usage of this method across the codebase:
The results show that saveImageToInternalStorage is actively used in three different files for critical functionality:
AppContactService.java: Stores contact profile imagesFileClientService.java: Handles file attachments and downloadsImageUtils.java: Contains the modified method
Let's verify if these callers have any error handling for empty strings:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all calls to saveImageToInternalStorage to assess impact
rg -l "saveImageToInternalStorage" | while read -r file; do
echo "=== $file ==="
rg -A 3 "saveImageToInternalStorage" "$file"
done
Length of output: 2092
Script:
#!/bin/bash
# Check error handling in caller methods
echo "=== AppContactService.java ==="
rg -B 3 -A 5 "saveImageToInternalStorage.*\{" kommunicate/src/main/java/com/applozic/mobicomkit/contact/AppContactService.java
echo -e "\n=== FileClientService.java ==="
rg -B 3 -A 5 "saveImageToInternalStorage.*\{" kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/FileClientService.java
# Also check if there are any TextUtils.isEmpty checks right after the calls
echo -e "\n=== Empty string checks ==="
rg -B 2 "TextUtils\.isEmpty.*getLocalImageUrl|TextUtils\.isEmpty.*getLocalImageUri|TextUtils\.isEmpty.*getTopicLocalImageUri" kommunicate/src/main/java/com/applozic/mobicomkit/
Length of output: 3061
| // File filePath; | ||
| // File dir = null; | ||
| // if (Environment.MEDIA_MOUNTED.equals(Environment.getExternalStorageState())) { | ||
| // String folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_OTHER_FILES_FOLDER; | ||
| // | ||
| // if (contentType.startsWith("image")) { | ||
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_IMAGES_FOLDER; | ||
| // } else if (contentType.startsWith("video")) { | ||
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_VIDEOS_FOLDER; | ||
| // } else if (contentType.equalsIgnoreCase(text_card)) { | ||
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_CONTACT_FOLDER; | ||
| // } | ||
| // if (isThumbnail) { | ||
| // folder = folder + MOBI_COM_THUMBNAIL_SUFIX; | ||
| // } | ||
| // File directory = context.getExternalFilesDir(null); | ||
| // if (directory != null) { | ||
| // dir = new File(directory.getAbsolutePath() + folder); | ||
| // if (!dir.exists()) { | ||
| // dir.mkdirs(); | ||
| // } | ||
| // } | ||
| // } else { | ||
| // ContextWrapper cw = new ContextWrapper(context); | ||
| // // path to /data/data/yourapp/app_data/imageDir | ||
| // dir = cw.getDir(IMAGE_DIR, Context.MODE_PRIVATE); | ||
| // } | ||
| // // Create image name | ||
| // //String extention = "." + contentType.substring(contentType.indexOf("/") + 1); | ||
| // filePath = new File(dir, fileName); | ||
| return null; |
There was a problem hiding this comment.
Critical: Handle null return value from getFilePath.
The method now returns null without providing an alternative implementation. This could lead to NullPointerException in calling methods like loadContactsvCard and loadThumbnailImage.
Consider implementing proper error handling:
public static File getFilePath(String fileName, Context context, String contentType, boolean isThumbnail) {
- return null;
+ throw new UnsupportedOperationException("External storage access has been deprecated");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // File filePath; | |
| // File dir = null; | |
| // if (Environment.MEDIA_MOUNTED.equals(Environment.getExternalStorageState())) { | |
| // String folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_OTHER_FILES_FOLDER; | |
| // | |
| // if (contentType.startsWith("image")) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_IMAGES_FOLDER; | |
| // } else if (contentType.startsWith("video")) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_VIDEOS_FOLDER; | |
| // } else if (contentType.equalsIgnoreCase(text_card)) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_CONTACT_FOLDER; | |
| // } | |
| // if (isThumbnail) { | |
| // folder = folder + MOBI_COM_THUMBNAIL_SUFIX; | |
| // } | |
| // File directory = context.getExternalFilesDir(null); | |
| // if (directory != null) { | |
| // dir = new File(directory.getAbsolutePath() + folder); | |
| // if (!dir.exists()) { | |
| // dir.mkdirs(); | |
| // } | |
| // } | |
| // } else { | |
| // ContextWrapper cw = new ContextWrapper(context); | |
| // // path to /data/data/yourapp/app_data/imageDir | |
| // dir = cw.getDir(IMAGE_DIR, Context.MODE_PRIVATE); | |
| // } | |
| // // Create image name | |
| // //String extention = "." + contentType.substring(contentType.indexOf("/") + 1); | |
| // filePath = new File(dir, fileName); | |
| return null; | |
| // File filePath; | |
| // File dir = null; | |
| // if (Environment.MEDIA_MOUNTED.equals(Environment.getExternalStorageState())) { | |
| // String folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_OTHER_FILES_FOLDER; | |
| // | |
| // if (contentType.startsWith("image")) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_IMAGES_FOLDER; | |
| // } else if (contentType.startsWith("video")) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_VIDEOS_FOLDER; | |
| // } else if (contentType.equalsIgnoreCase(text_card)) { | |
| // folder = "/" + Utils.getMetaDataValue(context, MAIN_FOLDER_META_DATA) + MOBI_COM_CONTACT_FOLDER; | |
| // } | |
| // if (isThumbnail) { | |
| // folder = folder + MOBI_COM_THUMBNAIL_SUFIX; | |
| // } | |
| // File directory = context.getExternalFilesDir(null); | |
| // if (directory != null) { | |
| // dir = new File(directory.getAbsolutePath() + folder); | |
| // if (!dir.exists()) { | |
| // dir.mkdirs(); | |
| // } | |
| // } | |
| // } else { | |
| // ContextWrapper cw = new ContextWrapper(context); | |
| // // path to /data/data/yourapp/app_data/imageDir | |
| // dir = cw.getDir(IMAGE_DIR, Context.MODE_PRIVATE); | |
| // } | |
| // // Create image name | |
| // //String extention = "." + contentType.substring(contentType.indexOf("/") + 1); | |
| // filePath = new File(dir, fileName); | |
| throw new UnsupportedOperationException("External storage access has been deprecated"); |
I commented out the getExternalDirectory useage as it is deprecated and I checked that out that we are not using it anymore. So, As of now I commented the code and test that if things is no flow is breaking in sanity. If sanity is done successfully then we can pick this as code cleanup task.
In the commented code I am sending the empty string as response which will raise the exception which is handelled by all the calling method of that function i.e
saveImageToInternalStorageThis issue was reported by the ImmueWeb in their VPAT report.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
loadContactsvCardmethod.Chores