feat: Remove request for storage permissions (#196)

It turns out Android does not need extra permissions for accessing files in external storage which are in the folder returned by getExternalFilesDir() https://developer.android.com/reference/android/content/Context.html#getExternalFilesDir(java.lang.String)
digidem · Aug 30, 2019 · 0c3c787 · 0c3c787
1 parent 9429c2d
commit 0c3c787
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 98 deletions.
diff --git a/__mocks__/bugsnag-react-native.js b/__mocks__/bugsnag-react-native.js
@@ -0,0 +1,6 @@
+export const Client = () => ({
+  leaveBreadcrumb: () => {},
+  notify: () => {}
+});
+
+export const Configuration = () => ({});
diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml
@@ -5,8 +5,6 @@
   <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" />
   <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
   <uses-permission android:name="android.permission.CAMERA" />
-  <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
-  <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
   <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
 
   <application android:name=".MainApplication" 

diff --git a/src/backend/index.js b/src/backend/index.js
@@ -63,10 +63,9 @@ status.startHeartbeat();
  * card. It is a shared folder that the user can access. The data folder
  * accessible through rnBridge.app.datadir() is not accessible by the user.
  *
- * The node process cannot request permission to read this folder and it does
- * not know the filepath. We need to wait for the React Native process to tell
- * us where the folder is. This code supports re-starting the server with a
- * different folder if necessary (we probably shouldn't do that)
+ * We need to wait for the React Native process to tell us where the folder is.
+ * This code supports re-starting the server with a different folder if
+ * necessary (we probably shouldn't do that)
  */
 rnBridge.channel.on("storagePath", path => {
   log("storagePath", path);

diff --git a/src/frontend/AppLoading.js b/src/frontend/AppLoading.js
@@ -42,9 +42,7 @@ class AppLoading extends React.Component<Props, State> {
     this.props.requestPermissions([
       PERMISSIONS.CAMERA,
       PERMISSIONS.ACCESS_COARSE_LOCATION,
-      PERMISSIONS.ACCESS_FINE_LOCATION,
-      PERMISSIONS.READ_EXTERNAL_STORAGE,
-      PERMISSIONS.WRITE_EXTERNAL_STORAGE
+      PERMISSIONS.ACCESS_FINE_LOCATION
     ]);
     this._timeoutId = setTimeout(() => {
       SplashScreen.hide();
@@ -55,7 +53,7 @@ class AppLoading extends React.Component<Props, State> {
   }
 
   componentDidUpdate() {
-    if (this.hasStoragePermission() && this.state.serverStatus == null) {
+    if (this.state.serverStatus == null) {
       api.startServer();
       this.setState({ serverStatus: Constants.STARTING });
     }
@@ -68,15 +66,6 @@ class AppLoading extends React.Component<Props, State> {
     SplashScreen.hide();
   }
 
-  hasStoragePermission() {
-    // $FlowFixMe - needs HOC type to be fixed
-    const { permissions } = this.props;
-    return (
-      permissions[PERMISSIONS.READ_EXTERNAL_STORAGE] === RESULTS.GRANTED &&
-      permissions[PERMISSIONS.WRITE_EXTERNAL_STORAGE] === RESULTS.GRANTED
-    );
-  }
-
   handleStatusChange = (serverStatus: ServerStatus) => {
     if (serverStatus === this.state.serverStatus) return;
     log("status change", serverStatus);

diff --git a/src/frontend/api.js b/src/frontend/api.js
@@ -1,6 +1,6 @@
 // @flow
 import "core-js/es/reflect";
-import { PixelRatio, PermissionsAndroid } from "react-native";
+import { PixelRatio } from "react-native";
 import ky from "ky";
 import nodejs from "nodejs-mobile-react-native";
 import RNFS from "react-native-fs";
@@ -163,46 +163,39 @@ export function Api({
       // The server might already be started - request current status
       nodejs.channel.post("request-status");
       bugsnag.leaveBreadcrumb("Starting Mapeo Core");
-      // The server requires read & write permissions for external storage
-      const serverStartPromise = PermissionsAndroid.requestMultiple([
-        PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE,
-        PermissionsAndroid.PERMISSIONS.WRITE_EXTERNAL_STORAGE
-      ])
-        .then(results => {
-          const permissionsGranted = Object.values(results).every(
-            r => r === PermissionsAndroid.RESULTS.GRANTED
-          );
-          if (!permissionsGranted)
-            throw new Error("Storage read and write permissions not granted");
-          bugsnag.leaveBreadcrumb("Mapeo Core permissions");
-          nodejs.start("loader.js");
-          // We know the node process has started as soon as we hear a status
-          return new Promise(resolve => nodejs.channel.once("status", resolve));
-        })
-        .then(() => {
-          bugsnag.leaveBreadcrumb("Mapeo Core started");
-          // Start monitoring for timeout
-          restartTimeout();
-          // As soon as we hear from the Node process, send the storagePath so
-          // that the server can start
-          nodejs.channel.post("storagePath", RNFS.ExternalDirectoryPath);
-          // Resolve once the server reports status as "LISTENING"
-          return onReady();
-        });
+      nodejs.start("loader.js");
+      const serverStartPromise = new Promise(resolve =>
+        nodejs.channel.once("status", resolve)
+      ).then(() => {
+        bugsnag.leaveBreadcrumb("Mapeo Core started");
+        // Start monitoring for timeout
+        restartTimeout();
+        // As soon as we hear from the Node process, send the storagePath so
+        // that the server can start
+        nodejs.channel.post("storagePath", RNFS.ExternalDirectoryPath);
+        // Resolve once the server reports status as "LISTENING"
+        return onReady();
+      });
+
       serverStartPromise.then(() =>
         bugsnag.leaveBreadcrumb("Mapeo Core ready")
       );
-      return promiseTimeout(
+
+      const serverStartTimeoutPromise = promiseTimeout(
         serverStartPromise,
         SERVER_START_TIMEOUT,
         "Server start timeout"
-      ).catch(e => {
+      );
+
+      serverStartTimeoutPromise.catch(e => {
         // We could get here when the timeout timer has not yet started and the
         // server status is still "STARTING", so we update the status to an
         // error
         onStatusChange(STATUS.ERROR);
         bugsnag.notify(e);
       });
+
+      return serverStartTimeoutPromise;
     },
 
     addServerStateListener: function addServerStateListener(

diff --git a/src/frontend/api.spec.js b/src/frontend/api.spec.js
@@ -1,6 +1,5 @@
 /* eslint-env jest/globals */
 import ky from "ky";
-import { PermissionsAndroid } from "react-native";
 import nodejs from "nodejs-mobile-react-native";
 import RNFS from "react-native-fs";
 import { Api, Constants } from "./api";
@@ -10,15 +9,6 @@ import { Api, Constants } from "./api";
 jest.mock("ky");
 jest.mock("nodejs-mobile-react-native");
 jest.mock("react-native-fs");
-jest.mock("PermissionsAndroid");
-
-function mockStoragePerms(result) {
-  return () =>
-    Promise.resolve({
-      [PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE]: result,
-      [PermissionsAndroid.PERMISSIONS.WRITE_EXTERNAL_STORAGE]: result
-    });
-}
 
 describe("Server startup", () => {
   test("Initialization sets up nodejs status listener", () => {
@@ -27,7 +17,7 @@ describe("Server startup", () => {
     expect(spy).toHaveBeenCalled();
   });
 
-  test("Start server with permissions granted", async () => {
+  test("Start server", async () => {
     expect.assertions(5);
     nodejs.start = jest.fn();
     // This mocks the initial heartbeat from the server
@@ -37,56 +27,29 @@ describe("Server startup", () => {
       handler(Constants.LISTENING)
     );
     nodejs.channel.post = jest.fn();
-    PermissionsAndroid.requestMultiple = jest.fn(
-      mockStoragePerms(PermissionsAndroid.RESULTS.GRANTED)
-    );
     const api = new Api({ baseUrl: "__URL__" });
     await expect(api.startServer()).resolves.toBeUndefined();
     expect(nodejs.start.mock.calls.length).toBe(1);
     expect(nodejs.start.mock.calls[0][0]).toBe("loader.js");
-    expect(nodejs.channel.post.mock.calls.length).toBe(1);
-    expect(nodejs.channel.post.mock.calls[0][1]).toBe(
+    expect(nodejs.channel.post.mock.calls.length).toBe(2);
+    expect(nodejs.channel.post.mock.calls[1][1]).toBe(
       RNFS.ExternalDirectoryPath
     );
   });
 
-  test("Start server permissions not granted rejects without trying to start server", async () => {
-    expect.assertions(3);
-    nodejs.start = jest.fn();
-    // This mocks the initial heartbeat from the server
-    nodejs.channel.once = jest.fn((_, handler) => handler());
-    // This mocks the server to immediately be in "Listening" state
-    nodejs.channel.addListener = jest.fn((_, handler) =>
-      handler(Constants.LISTENING)
-    );
-    nodejs.channel.post = jest.fn();
-    PermissionsAndroid.requestMultiple = jest.fn(
-      mockStoragePerms(PermissionsAndroid.RESULTS.DENIED)
-    );
-    const api = new Api({ baseUrl: "__URL__" });
-    await expect(api.startServer()).rejects.toThrow(
-      "Storage read and write permissions not granted"
-    );
-    expect(nodejs.start.mock.calls.length).toBe(0);
-    expect(nodejs.channel.post.mock.calls.length).toBe(0);
-  });
-
   test("Start server timeout", async () => {
     jest.useFakeTimers();
     expect.assertions(4);
     nodejs.start = jest.fn();
     nodejs.channel.once = jest.fn();
     nodejs.channel.addListener = jest.fn();
     nodejs.channel.post = jest.fn();
-    PermissionsAndroid.requestMultiple = jest.fn(
-      mockStoragePerms(PermissionsAndroid.RESULTS.GRANTED)
-    );
     const api = new Api({ baseUrl: "__URL__" });
     process.nextTick(() => jest.runAllTimers());
     await expect(api.startServer()).rejects.toThrow("Server start timeout");
     expect(nodejs.start.mock.calls.length).toBe(1);
     expect(nodejs.start.mock.calls[0][0]).toBe("loader.js");
-    expect(nodejs.channel.post.mock.calls.length).toBe(0);
+    expect(nodejs.channel.post.mock.calls.length).toBe(1);
   });
 });
 
@@ -217,9 +180,6 @@ function startServer() {
   // This mocks the initial heartbeat from the server
   nodejs.channel.once = jest.fn((_, handler) => handler());
   nodejs.channel.post = jest.fn();
-  PermissionsAndroid.requestMultiple = jest.fn(
-    mockStoragePerms(PermissionsAndroid.RESULTS.GRANTED)
-  );
   nodejs.channel.addListener = jest.fn((_, handler) => {
     handler(Constants.LISTENING);
     serverStatus = handler;

diff --git a/src/frontend/context/PermissionsContext.js b/src/frontend/context/PermissionsContext.js
@@ -14,9 +14,7 @@ export type PermissionResult = "granted" | "denied" | "never_ask_again";
 type PermissionType =
   | "android.permission.CAMERA"
   | "android.permission.ACCESS_FINE_LOCATION"
-  | "android.permission.ACCESS_COARSE_LOCATION"
-  | "android.permission.READ_EXTERNAL_STORAGE"
-  | "android.permission.WRITE_EXTERNAL_STORAGE";
+  | "android.permission.ACCESS_COARSE_LOCATION";
 
 export type PermissionsType = {|
   [PermissionType]: PermissionResult
@@ -31,9 +29,7 @@ export const RESULTS: { [string]: PermissionResult } = {
 export const PERMISSIONS: { [string]: PermissionType } = {
   CAMERA: "android.permission.CAMERA",
   ACCESS_FINE_LOCATION: "android.permission.ACCESS_FINE_LOCATION",
-  ACCESS_COARSE_LOCATION: "android.permission.ACCESS_COARSE_LOCATION",
-  READ_EXTERNAL_STORAGE: "android.permission.READ_EXTERNAL_STORAGE",
-  WRITE_EXTERNAL_STORAGE: "android.permission.WRITE_EXTERNAL_STORAGE"
+  ACCESS_COARSE_LOCATION: "android.permission.ACCESS_COARSE_LOCATION"
 };
 
 type RequestPermissions = (type: PermissionType | PermissionType[]) => any;