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.

Sign up for GitHub

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

Remove-External-File-Directory Permission #554

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Remove-External-File-Directory Permission #554

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8bf7da
Commented the code which is acessing tge external file dir
prateek-kommunicate Oct 1, 2024
47b2cbc
Added the exception instead of passing empty string.
prateek-kommunicate Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 40 additions & 42 deletions kommunicate/src/main/java/com/applozic/mobicomkit/api/attachment/FileClientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
package com.applozic.mobicomkit.api.attachment;

import android.content.Context;
import android.content.ContextWrapper;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.media.ThumbnailUtils;
import android.net.Uri;
import android.os.Environment;
import android.os.Handler;
import android.provider.MediaStore;
import android.text.TextUtils;
Expand Down Expand Up @@ -43,25 +41,25 @@
public class FileClientService extends MobiComKitClientService {

//Todo: Make the base folder configurable using either strings.xml or properties file
public static final String MOBI_COM_IMAGES_FOLDER = "/image";
public static final String MOBI_COM_VIDEOS_FOLDER = "/video";
public static final String MOBI_COM_CONTACT_FOLDER = "/contact";
public static final String MOBI_COM_OTHER_FILES_FOLDER = "/other";
public static final String MOBI_COM_THUMBNAIL_SUFIX = "/.Thumbnail";
// public static final String MOBI_COM_IMAGES_FOLDER = "/image";
// public static final String MOBI_COM_VIDEOS_FOLDER = "/video";
// public static final String MOBI_COM_CONTACT_FOLDER = "/contact";
// public static final String MOBI_COM_OTHER_FILES_FOLDER = "/other";
// public static final String MOBI_COM_THUMBNAIL_SUFIX = "/.Thumbnail";
public static final String FILE_UPLOAD_URL = "/rest/ws/aws/file/url";
public static final String IMAGE_DIR = "image";
// public static final String IMAGE_DIR = "image";
public static final String AL_UPLOAD_FILE_URL = "/rest/ws/upload/file";
public static final String CUSTOM_STORAGE_SERVICE_END_POINT = "/rest/ws/upload/image";
// public static final String S3_SIGNED_URL_END_POINT = "/rest/ws/upload/file";
public static final String S3_SIGNED_URL_END_POINT = "/rest/ws/upload/image";
public static final String S3_SIGNED_URL_PARAM = "aclsPrivate";
public static final String THUMBNAIL_URL = "/files/";
// public static final String THUMBNAIL_URL = "/files/";
private static final int MARK = 1024;
private static final String TAG = "FileClientService";
private static final String MAIN_FOLDER_META_DATA = "main_folder_name";
// private static final String MAIN_FOLDER_META_DATA = "main_folder_name";
private HttpRequestUtils httpRequestUtils;
private MobiComKitClientService mobiComKitClientService;
private static final String text_card = "text/x-vCard";
// private static final String text_card = "text/x-vCard";

public FileClientService(Context context) {
super(context);
Expand All @@ -70,37 +68,37 @@ public FileClientService(Context context) {
}

public static File getFilePath(String fileName, Context context, String contentType, boolean isThumbnail) {
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 filePath;
// 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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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");

}

public static File getFilePath(String fileName, Context context, String contentType) {
Expand Down
34 changes: 17 additions & 17 deletions kommunicate/src/main/java/com/applozic/mobicommons/commons/image/ImageUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,23 +159,23 @@ public static Bitmap getBitMapFromLocalPath(String imageLocalPath) {


public static String saveImageToInternalStorage(File file, Bitmap bitmapImage) {
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 file.getAbsolutePath();
// 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 "";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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

⚠️ Potential issue

Returning empty string breaks image storage functionality across multiple components

The verification reveals that returning an empty string from saveImageToInternalStorage is problematic because:

  1. The method is used in critical image storage flows:

    • Contact profile images in AppContactService
    • File attachments in FileClientService
    • Topic images in conversation flows

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

  3. 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:

  1. The method signature suggests it should return a file path, but now returns an empty string
  2. Parameters file and bitmapImage are now unused
  3. 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:

  1. AppContactService.java: Stores contact profile images
  2. FileClientService.java: Handles file attachments and downloads
  3. ImageUtils.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

}


Expand Down