Issue #1403 - On iOS, bypass 'always' permission by default

[781flyingdutchman]

To prevent users getting rejected by Apple for not including NSLocationAlwaysAndWhenInUseUsageDescription even if they never require background location updates.

This PR makes the compilation of the code to request 'always' permission conditional on the flag PERMISSION_LOCATION_ALWAYS being set in XCode under 'Preprocessor Macros'. If not set (the default) then the permission request for 'always' is not included in the binary, therefore not triggering a complaint by Apple.

The request for 'always' (background) location updates is rare, so it is reasonable to have this turned off by default, and require some effort by the developer to activate this.

Changes:

• PermissionHandler.m (add conditional compile)
• README.md (modified iOS permission section to explain default behavior and how to set the PERMISSION_LOCATION_ALWAYS flag if needed)

I did not update the CHANGELOG as I imagine you may want to combine this with something else.

Pre-launch Checklist

• I made sure the project builds.
• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is does not need version changes.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I rebased onto main.
• I added new tests to check the change I am making, or this PR does not need tests.
• I made sure all existing and new tests are passing.
• I ran dart format . and committed any changes.
• I ran flutter analyze and fixed any errors.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b311084) 94.73% compared to head (c933009) 100.00%.
Report is 31 commits behind head on main.

❗ Current head c933009 differs from pull request most recent head 10923a3. Consider uploading reports for the commit 10923a3 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #1404      +/-   ##
===========================================
+ Coverage   94.73%   100.00%   +5.26%     
===========================================
  Files           4         1       -3     
  Lines         114        30      -84     
===========================================
- Hits          108        30      -78     
+ Misses          6         0       -6     

Flag	Coverage Δ
unittests	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mvanbeusekom]

Hi @781flyingdutchman,

Apologies for the belated reply, and thank you very much for taking the time to submit this PR.

I really like the idea of adding this option, however I was wondering if it is possible to flip the behaviour. The current implementation requires developers to explicitly turn the PERMISSION_LOCATION_ALWAYS on, which is a breaking change.

Would it be possible to reverse this behaviour and have developers that do not require background permissions explicitly turn it of? This would keep the default behaviour intact and prevents us from introducing a breaking change.

Also could you update the pubspec.yaml with a version update (a minor increase would be sufficient) and add a matching entry to the CHANGELOG.md file?

[mp-jojo]

This is an awesome change! I agree that breaking changes should be avoided, and I don't really care if I need to do some additional steps to avoid the "always" permission on iOS. I am a bit surprised that it currently isn't possible, and I'm even more surprised that this PR is discussed 1 day after I realised that I need this change to happen :)

[781flyingdutchman]

@mvanbeusekom happy to make that change, stay tuned

[781flyingdutchman]

@mvanbeusekom I have made the following changes:

• Merged the latest main branch
• Reversed the effect of the flag, and renamed it to BYPASS_PERMISSION_LOCATION_ALWAYS, such that if it is not set the behavior is the same as it was before (no breaking change)
• Updated pubspec.yaml for the plugin and for the _apple module (incremented minor)
• Updated README to reflect the new flag behavior
• Updated CHANGELOG

I have to say I am a bit confused by the various versions of the example app in different directories, so had a hard time testing this time around. However, the latest change is minor (just added ! before the new flag name) I am sure it should work, but please test before releasing!

[mvanbeusekom]

Thanks for the changes @781flyingdutchman, they look good. I have two small points of feedback which would be great if you can have a look at.

[mvanbeusekom]

The markdown in the README supports displaying formatted code by wrapping a certain section with backticks. In this case we can use it around the "'BYPASS_PERMISSION_LOCATION_ALWAYS=1'" keywords, which will render it as code. That means it will also not be necessary to inform developers to leave out the quotes (since the backticks are not rendered):

Suggested change

	If you don't need to receive updates when your app is in the background, then add a compiler flag as follows: in XCode, click on Pods, choose the Target 'geolocator_apple', choose Build Settings, in the search box look for 'Preprocessor Macros' then add the 'BYPASS_PERMISSION_LOCATION_ALWAYS=1' flag (without the quotes).
	If you don't need to receive updates when your app is in the background, then add a compiler flag as follows: in XCode, click on Pods, choose the Target 'geolocator_apple', choose Build Settings, in the search box look for 'Preprocessor Macros' then add the `BYPASS_PERMISSION_LOCATION_ALWAYS=1` flag.

[mvanbeusekom]

This can be moved to the CHANGELOG.md file of the geolocator_apple package. Since the geolocator is setup according to the federated plugin architecture, each platform has it's own package that is published to pub.dev (although the platform specific packages are marked unlisted, they are still published). This means we have to keep track changes for each package in each of the CHANGELOG.md files.

Instead it would be nice if this could say something a long the lines of:

- Adds a description to the README on how to add the `BYPASS_PERMISSION_LOCATION_ALWAYS` preprocessor to bypass the need for adding the `NSLocationAlwaysUsageDescription` to the `Info.plist`.

[mp-jojo]

I thought I would try this PR out, so I pointed my pubspec.yaml to the fork that @781flyingdutchman have created like this:

dependencies:
  geolocator:
    git:
      url: [email protected]:781flyingdutchman/flutter-geolocator.git
      ref: always_permission_bypass
      path: geolocator

After this I did a flutter clean and flutter build ios, and then followed the instructions on how to add the preprocessor flag:

However I still get the warning-email from Apple. Have I done something wrong in the changes above?

I also realised a problem with this solution, as soon as I run pod install, the changes made to "Preprocessor Macros" disappears. Another problem is that we don't version control our dependencies, so the changes I make to Pods.xcodeproj can't be comitted. Maybe this can be solved by some script that always makes the change before compiling.

[781flyingdutchman]

@mp-jojo Have you tried deleting the podspec.lock file and the POS directory, and then applying the preprocessor macro? This forces a rebuild and may solve the problem. If so, I will add that to the readme.
You can check if the flag is active by going into the code on XCode (with your app as the target). Under the Pods target, Development Pods, follow '..' a few times to get to your app then .symlinks, plugins etc to iOS, Classes, Handlers, Permission handler.m. The code that is surrounded by flags should show dimmed if the flag is set such that it is skipped. Again, I had a hard time getting the example app to use the plugin code last time I tried, and I thought this was due to the confusing nesting of example apps but perhaps there is a bigger issue? It worked fine for me before I flipped the flag meaning.

I am not aware of a way to script this. In Swift you can change the Pod file but that doesn't work for C flags. If you know of a way, please let me know!

[mp-jojo]

@mp-jojo Have you tried deleting the podspec.lock file and the POS directory, and then applying the preprocessor macro? This forces a rebuild and may solve the problem. If so, I will add that to the readme. You can check if the flag is active by going into the code on XCode (with your app as the target). Under the Pods target, Development Pods, follow '..' a few times to get to your app then .symlinks, plugins etc to iOS, Classes, Handlers, Permission handler.m. The code that is surrounded by flags should show dimmed if the flag is set such that it is skipped. Again, I had a hard time getting the example app to use the plugin code last time I tried, and I thought this was due to the confusing nesting of example apps but perhaps there is a bigger issue? It worked fine for me before I flipped the flag meaning.

I am not aware of a way to script this. In Swift you can change the Pod file but that doesn't work for C flags. If you know of a way, please let me know!

I realised that the problem might have been that geolocator_apple is a "sub dependency" of geolocator, so it probably fetched all sub-dependencies from the official repo. I added this section to my pubspec.yaml:

dependency_overrides:
  geolocator_apple:
    git:
      url: [email protected]:781flyingdutchman/flutter-geolocator.git
      ref: always_permission_bypass
      path: geolocator_apple

I am however not sure if that solved it, or the fact that I deleted ~/Library/Caches/CocoaPods, Pods and Podfile.lock. Regardless, I have now successfully uploaded a build of my app to testflight that doesn't warn about NSLocationAlwaysAndWhenInUseUsageDescription, so it works as intended!

I also added the below script to the bottom of my Podfile, so it will apply this flag to Preprocessor Macros on each build, also seems to work good!

post_install do |installer|
  installer.pods_project.targets.each do |target|
    if target.name == "geolocator_apple"
      target.build_configurations.each do |config|
        config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] ||= ['$(inherited)', 'BYPASS_PERMISSION_LOCATION_ALWAYS=1']
      end
    end
  end
end

[mvanbeusekom]

@mp-jojo, thank you for testing and glad it finally worked out.

@781flyingdutchman, in the permission_handler plugin we also add preprocessor definitions to projects using the script mentioned by @mp-jojo (see iOS section of the readme). I think it might be nice to mention this as an alternative in the README of the geolocator package (next to the current solution you already documented). Would this be something you could add to this PR?

[781flyingdutchman]

Absolutely, that's how I've done it in background_downloader - I just didn't know how to do that for Objective C.
I'll add these changes

[781flyingdutchman]

Updated the README, CHANGELOG and Podfile as suggested

[mvanbeusekom]

LGTM, thanks @781flyingdutchman for all your work and improvements