From 3b129a6ea4d0e52fd6d47dcd44187b4d9a1721b5 Mon Sep 17 00:00:00 2001 From: Josephine Lim Date: Wed, 18 Apr 2018 21:30:13 +1000 Subject: [PATCH] Fix security exception (Nearby places not loading after permissions granted) (#1440) * Update changelog.md * Versioning for v2.7.0 * Add logging to onPermissionsRequestResult * Request location updates onStatusChanged * Copy onResume() actions into onPermissionsRequestResult * Added getLastKnownLocation method and hooked it up to refreshView * Remove unnecessary calls, add more logging * Add check to prevent NPE * Check that curLatLng exists before getting Mapbox instance * Moar logging * Make curLatLang clearer * Not a good hack - put curLatLang into the bundle separately * Add TODO * Rename variables for clarity * Check for Network Provider as well, tidy up getLKL() * Add Javadocs * Remove unnecessary method in onStatusChanged * Add checkGPS comment * Remove unnecessary logs * Add TODO * Call bundle.clear() before inserting CurLatLng --- .../location/LocationServiceManager.java | 20 ++++++++++- .../nrw/commons/nearby/NearbyActivity.java | 36 +++++++++++++------ .../commons/nearby/NearbyListFragment.java | 7 ++-- .../nrw/commons/nearby/NearbyMapFragment.java | 11 ++++-- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java index f9a171461..73ded852f 100644 --- a/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java +++ b/app/src/main/java/fr/free/nrw/commons/location/LocationServiceManager.java @@ -79,6 +79,23 @@ public class LocationServiceManager implements LocationListener { Manifest.permission.ACCESS_FINE_LOCATION); } + /** + * Gets the last known location in cases where there wasn't time to register a listener + * (e.g. when Location permission just granted) + * @return last known LatLng + */ + public LatLng getLKL() { + if (ContextCompat.checkSelfPermission(context, Manifest.permission.ACCESS_FINE_LOCATION) == PackageManager.PERMISSION_GRANTED) { + Location lastKL = locationManager.getLastKnownLocation(LocationManager.GPS_PROVIDER); + if (lastKL == null) { + lastKL = locationManager.getLastKnownLocation(LocationManager.NETWORK_PROVIDER); + } + return LatLng.from(lastKL); + } else { + return null; + } + } + public LatLng getLastLocation() { if (lastLocation == null) { return null; @@ -249,6 +266,7 @@ public class LocationServiceManager implements LocationListener { public enum LocationChangeType{ LOCATION_SIGNIFICANTLY_CHANGED, //Went out of borders of nearby markers LOCATION_SLIGHTLY_CHANGED, //User might be walking or driving - LOCATION_NOT_CHANGED + LOCATION_NOT_CHANGED, + PERMISSION_JUST_GRANTED } } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java index 176c45c36..b75d179c4 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java @@ -13,14 +13,12 @@ import android.support.design.widget.BottomSheetBehavior; import android.support.v4.app.FragmentTransaction; import android.support.v7.app.AlertDialog; -import android.util.Log; import android.view.Menu; import android.view.MenuInflater; import android.view.MenuItem; import android.view.View; import android.widget.LinearLayout; import android.widget.ProgressBar; -import android.widget.Toast; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -81,6 +79,7 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp private final String NETWORK_INTENT_ACTION = "android.net.conn.CONNECTIVITY_CHANGE"; private BroadcastReceiver broadcastReceiver; + private LatLng lastKnownLocation; @Override protected void onCreate(Bundle savedInstanceState) { @@ -158,7 +157,11 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp switch (requestCode) { case LOCATION_REQUEST: { if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED) { - refreshView(LocationServiceManager.LocationChangeType.LOCATION_SIGNIFICANTLY_CHANGED); + Timber.d("Location permission granted, refreshing view"); + //Still need to check if GPS is enabled + checkGps(); + lastKnownLocation = locationManager.getLKL(); + refreshView(LocationServiceManager.LocationChangeType.PERMISSION_JUST_GRANTED); } else { //If permission not granted, go to page that says Nearby Places cannot be displayed hideProgressBar(); @@ -347,21 +350,33 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp } curLatLang = lastLocation; + if (locationChangeType.equals(LocationServiceManager.LocationChangeType.PERMISSION_JUST_GRANTED)) { + curLatLang = lastKnownLocation; + } + if (curLatLang == null) { Timber.d("Skipping update of nearby places as location is unavailable"); return; } - if (locationChangeType - .equals(LocationServiceManager.LocationChangeType.LOCATION_SIGNIFICANTLY_CHANGED)) { + if (locationChangeType.equals(LocationServiceManager.LocationChangeType.LOCATION_SIGNIFICANTLY_CHANGED) + || locationChangeType.equals(LocationServiceManager.LocationChangeType.PERMISSION_JUST_GRANTED)) { progressBar.setVisibility(View.VISIBLE); + + //TODO: This hack inserts curLatLng before populatePlaces is called (see #1440). Ideally a proper fix should be found + Gson gson = new GsonBuilder() + .registerTypeAdapter(Uri.class, new UriSerializer()) + .create(); + String gsonCurLatLng = gson.toJson(curLatLang); + bundle.clear(); + bundle.putString("CurLatLng", gsonCurLatLng); + placesDisposable = Observable.fromCallable(() -> nearbyController .loadAttractionsFromLocation(curLatLang)) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(this::populatePlaces); - } else if (locationChangeType - .equals(LocationServiceManager.LocationChangeType.LOCATION_SLIGHTLY_CHANGED)) { + } else if (locationChangeType.equals(LocationServiceManager.LocationChangeType.LOCATION_SLIGHTLY_CHANGED)) { Gson gson = new GsonBuilder() .registerTypeAdapter(Uri.class, new UriSerializer()) .create(); @@ -384,14 +399,14 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp if (placeList.size() == 0) { ViewUtil.showSnackbar(findViewById(R.id.container), R.string.no_nearby); } - - bundle.clear(); + bundle.putString("PlaceList", gsonPlaceList); - bundle.putString("CurLatLng", gsonCurLatLng); + //bundle.putString("CurLatLng", gsonCurLatLng); bundle.putString("BoundaryCoord", gsonBoundaryCoordinates); // First time to init fragments if (nearbyMapFragment == null) { + Timber.d("Init map fragment for the first time"); lockNearbyView(true); setMapFragment(); setListFragment(); @@ -399,6 +414,7 @@ public class NearbyActivity extends NavigationBaseActivity implements LocationUp lockNearbyView(false); } else { // There are fragments, just update the map and list + Timber.d("Map fragment already exists, just update the map and list"); updateMapFragment(false); updateListFragment(); } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java index 4a2be8476..1be2a8689 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java @@ -82,8 +82,11 @@ public class NearbyListFragment extends DaggerFragment { } public void updateNearbyListSignificantly() { - adapterFactory.updateAdapterData(getPlaceListFromBundle(bundleForUpdates), - (RVRendererAdapter) recyclerView.getAdapter()); + try { + adapterFactory.updateAdapterData(getPlaceListFromBundle(bundleForUpdates), (RVRendererAdapter) recyclerView.getAdapter()); + } catch (NullPointerException e) { + Timber.e("Null pointer exception from calling recyclerView.getAdapter()"); + } } private List getPlaceListFromBundle(Bundle bundle) { diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java index a2ce12444..4b0ff5cc3 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java @@ -126,6 +126,7 @@ public class NearbyMapFragment extends DaggerFragment { @Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); + Timber.d("Nearby map fragment created"); controller = new ContributionController(this); directUpload = new DirectUpload(this, controller); @@ -151,9 +152,11 @@ public class NearbyMapFragment extends DaggerFragment { getActivity()); boundaryCoordinates = gson.fromJson(gsonBoundaryCoordinates, gsonBoundaryCoordinatesType); } - Mapbox.getInstance(getActivity(), - getString(R.string.mapbox_commons_app_token)); - MapboxTelemetry.getInstance().setTelemetryEnabled(false); + if (curLatLng != null) { + Mapbox.getInstance(getActivity(), + getString(R.string.mapbox_commons_app_token)); + MapboxTelemetry.getInstance().setTelemetryEnabled(false); + } setRetainInstance(true); } @@ -161,7 +164,9 @@ public class NearbyMapFragment extends DaggerFragment { public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) { + Timber.d("onCreateView called"); if (curLatLng != null) { + Timber.d("curLatLng found, setting up map view..."); setupMapView(savedInstanceState); }