Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-24142] iOS: Custom Ti.Facebook entitlements-file overrides CLI-generated entitlements

GitHub Issuen/a
TypeBug
PriorityHigh
StatusClosed
ResolutionFixed
Resolution Date2016-12-19T13:19:56.000+0000
Affected Version/sRelease 6.0.0, Release 5.5.1
Fix Version/sRelease 6.0.1
ComponentsiOS, Tooling
Labelsaps, entitlements, facebook, keychain-access, team-id
ReporterHans Knöchel
AssigneeChris Barber
Created2016-11-12T10:05:30.000+0000
Updated2016-12-19T13:19:59.000+0000

Description

Comments

  1. Hans Knöchel 2016-11-12

    *Workaround*: Copy the contents of the generated entitlements file in build/iphone to your own file and add the keychain-access-groups key to it afterwards. Then place that file in the root of your project as suggested [here](https://github.com/appcelerator-modules/ti.facebook/blob/3feee7d77ed9eef20936d173292c9e21089c1361/apidoc/Facebook.yml#L17-L34).
  2. Hans Knöchel 2016-11-12

    Ok, so here is my proposal: PR (titanium_mobile/master): https://github.com/appcelerator/titanium_mobile/pull/8603 PR (ti.facebook/master): https://github.com/appcelerator-modules/ti.facebook/pull/75 It basically checks if we use the ti.facebook iOS module and if so, it injects the keychain-access-groups key. This key will already be used for device-builds, but different to our push-notifications, we already need the capability in the Simulator when using Ti.Facebook (because Facebook uses the keychain to store private data, probably the access-token). But as we don't have the appPrefix from the provisioning profile in the Simulator, we cannot grab it from there. -So I added a check for the <team-id/> that we already use for watchOS-enabled apps. If it's there -> great, use it. If not, throw an error because Ti.Facebook won't work.- I updated the PR to use the $(AppIdentifierPrefix) flag instead that will take care of the Simulator keychain. Also, I'm wondering why we couldn't use this for device-builds as well - can the team-id from the provisioning be different from what Xcode uses when processing the flag? I don't think so. But anyway, both will work and the above PR has been tested for the following use-cases: - Ti.Facebook not included -> Skip - Ti.Facebook included, but not the iOS one -> Skip - Ti.Facebook included, run on device -> Proper entitlements generated in build/iphone - Ti.Facebook included, run on simulator -> Proper entitlements generated in build/iphone - Ti.Facebook included, push used, run on device -> Proper entitlements generated in build/iphone - Ti.Facebook included, push used, run on simulator -> Proper entitlements generated in build/iphone Code-style to be discussed, [~cbarber] is the man to check! Backport coming as soon as the master-PR is accepted. *EDIT*: And if we accept the PR, we can also remove the manual notes from the Ti.Facebook docs and delelopers will be happy because they don't need to curate multiple entitlements files. Test-case:
       var fb = require('facebook');
       fb.initialize();
       fb.permissions = ['email'];
       
       var win = Ti.UI.createWindow({
           backgroundColor: "#fff"
       });
       
       var btn = Ti.UI.createButton({
           title: "Trigger login"
       });
       
       fb.addEventListener("logout", function(e) {
           Ti.API.info("Logged out!");
           Ti.API.info(e);
       });
       
       fb.addEventListener("login", function(e) {
           Ti.API.info("Logged in!");
           Ti.API.info(e);
       
           if (e.success) {
               alert("Logged In!");    
           }
       })
       
       btn.addEventListener("click", function() {
           fb.logout();
           fb.authorize();
       });
       
       win.add(btn);
       win.open();
       
  3. Chris Barber 2016-11-14

    After giving the titanium_mobile PR much consideration, I think that we should NOT accept it. It tightly couples an optional module into the already overly complicated iOS build system. Instead, I propose we add a new "build.ios.writeEntitlements" hook to the iOS build in the _embedCapabilitiesAndWriteEntitlementsPlist() function. This function is called for both the Titanium app's entitlements file and each iOS Extension's entitlements file, so we would need to pass in a flag to indicate which context so that it can create the correct hook name "build.ios.writeEntitlements" or "build.ios.writeExtentionEntitlements". It was easier for me to just code it then the tell you how to do it. :) PR: https://github.com/appcelerator/titanium_mobile/pull/8606 Then in the Facebook module, add a hooks/facebook.js that hooks into the "build.ios.writeEntitlements" event and adds the missing keychain-access-groups. It would look like:
       'use strict';
       
       exports.id = 'com.appcelerator.facebook';
       
       exports.cliVersion = '>=3.2';
       
       exports.init = function init(logger, config, cli, appc) {
       	cli.on('build.ios.writeEntitlements', {
       		pre: function (data, finished) {
       			var applicationIdentifier = '$(AppIdentifierPrefix)' + this.tiapp.id;
       			var plist = data.args[0];
       
       			Array.isArray(plist['keychain-access-groups']) || (plist['keychain-access-groups'] = []);
       			if (!plist['keychain-access-groups'].some(function (id) { return id === applicationIdentifier; })) {
       				plist['keychain-access-groups'].push(applicationIdentifier);
       			}
       
       			finished();
       		}
       	});
       };
       
    Much cleaner integration. I should note that this code has not been tested, so give it a quick test.
  4. Hans Knöchel 2016-11-16

    [~cbarber] Will this be ready for 6.0.1? It needs to be well-tested against many use-cases. Thx!
  5. Chris Barber 2016-11-16

    [~hansknoechel] Well, I wasn't planning on allocating any more time to this ticket. I agree, it needs to be well-tested.
  6. Hans Knöchel 2016-11-16

    The problem that I'm having with this is that it needs both module and SDK-updates as well as TIMOB-24041 resolved. So either use the other solution for 6.0.1 and come up with the refactored one in 6.1.0 or finish both tickets in this sprint. More worried about the available resources for this issue, that's why I came up with my workaround.
  7. Chris Barber 2016-11-16

    Well, separate out the module hook into a new MOD ticket. That should take 5 minutes. Then let's get the PR for this ticket tested and merged. That should take an hour. Then lets fix TIMOB-24041, which should take about 2 hours. Then I've already written the hook for the Facebook module, just need to drop that in and test it, then merge and issue a PR for the updated iOS Facebook module. Maybe 2 hours of work. Easy peasy.
  8. Hans Knöchel 2016-11-17

    Created MOD-2313 for adding the CLI-hook.
  9. Chris Barber 2016-11-17

    I confirmed the new hooks are fired and the entitlements can be manipulated. The new hooks are build.ios.writeEntitlements and build.ios.writeExtensionEntitlements. TiSDK master PR: https://github.com/appcelerator/titanium_mobile/pull/8606 TiSDK 6_0_X PR: https://github.com/appcelerator/titanium_mobile/pull/8622
  10. Hans Knöchel 2016-11-18

    The PR works very well, tested together with MOD-2313 and the hook is called correctly, the capabilities are updated as expected.
  11. Eric Wieber 2016-12-01

    [~cbarber], [~hansknoechel], I am seeing some unexpected behavior when building with a custom entitlements file in /platform/ios. On first/clean builds, the custom file looks to be copied to the build folder. On subsequent builds, the entitlements file in the build folder is overwritten with the generated file. I would expect the same file in the build folder on each build, without any modifications to the project. Is this accurate? Tested with SDK 6.0.1.v20161130023500 and Facebook Module 5.2.9
  12. Eric Wieber 2016-12-01

    The issue has been narrowed down to the Module hook, so moving the conversation to that ticket: MOD-2313
  13. Chris Barber 2016-12-02

    [~ewieber] So, here's the deal. Custom entitlements files MUST be named Entitlements.plist and it must go in the root of your Titanium app. This custom entitlements file is read in, processed, and written to build/iphone/<app name>.entitlements. However, this is not what you did. You put a <app name>.entitlements file in the platform/ios directory. Your custom entitlements file will be copied over the generated <app name>.entitlements. When a subsequent build occurs, it will generate the build/iphone/<app name>.entitlements file again. However, this time when it copies the platform/ios/<app name>.entitlements file, it will find the entry from the previous build in the manifest and check the timestamps and hash of the platform/ios/<app name>.entitlements against what was copied in the previous build. Since they are the same, the platform/ios/<app name>.entitlements file is NOT copied. The problem is the build has written a newly generated <app name>.entitlements and the entry in the previous build manifest is now stale. Probably the easiest thing to do is delete the entry from the previous build manifest when the generated <app name>.entitlements is created. When the iOS build goes to copy platform/ios/<app name>.entitlements, there will be no entry and it will force copy the custom entitlements file. The fix should be done in the _embedCapabilitiesAndWriteEntitlementsPlist() function, after the this.unmarkBuildDirFile(dest); call. We need to do something like this amazing and untested code:
        if (this.previousBuildManifest.files) {
        	var rel = path.relative(this.buildDir, dest);
        	['ios', 'iphone'].forEach(function (dir) {
        		var file = path.join(this.projectDir, 'platform', dir, rel);
        		if (fs.existsSync(path.join(this.projectDir, file))) {
        			delete this.previousBuildManifest.files[file];
        		}
        	});
        }
        
    Note that we have to be careful since the _embedCapabilitiesAndWriteEntitlementsPlist() function is also called for entitlements files from extensions and watch apps, hence the relative path handling.
  14. Eric Wieber 2016-12-02

    [~cbarber], what you explained makes sense and maybe I need to better clarify the entitlements files we are talking about. There are custom entitlements.plist files and custom .entitlements files at work. We recommend to use the latter in TIMOB-23897 and is what uncovered this issue. Anyway, can we expect a PR for your fix or would you like to break it off into its own ticket?
  15. Chris Barber 2016-12-02

    [~ewieber] I'll just reopen this ticket since it's a "bug" with the fix for this ticket.
  16. Chris Barber 2016-12-02

    TiSDK master PR: https://github.com/appcelerator/titanium_mobile/pull/8653 TiSDK 6_0_X PR: https://github.com/appcelerator/titanium_mobile/pull/8654 To test:

    Create the platform/ios directory

    Build an iOS app for simulator ti build -p ios --build-only

    Copy the build/iphone/<app name>.entitlements to platform/ios/<app name>.entitlements

    Modify the platform/ios/<app name>.entitlements by adding <key>foo</key><string>bar</string>

    Clean the project ti clean

    Build the iOS app for simulator ti build -p ios --build-only

    cat build/iphone/<app name>.entitlements and it should contain the foo key

    Build the app again ti build -p ios --build-only

    cat build/iphone/<app name>.entitlements and it should contain the foo key

  17. Hans Knöchel 2016-12-02

    Tested with both platform/ios and platform/iphone, the custom one is used after this change, so the patch is working.
  18. Eric Wieber 2016-12-02

    Verified fixed, using: MacOS 10.12 (16A323) Ti SDK 6.0.1.v20161202124626 Appc NPM 4.2.8 Appc CLI 6.0.0 Alloy 1.9.4 Xcode 8.1 (8B62) The correct entitlements are present after building with a custom entitlements.plist, .entitlements, or neither file present in the app. keychain-access-groups is populated and with the exception of an issue where the key has multiple values (MOD-2313) the file looks correct.

JSON Source