[resign] fix entitlements merging when changing team while resigning #18713

silvervest · 2021-05-19T05:28:24Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

These are the changes from #12131 updated to master and with merge conflicts resolved. Please see that issue for more in-depth detail.

This was resubmitted again due to lose head repository branch, but is the same changes.

Description

Testing Steps

supersedes #12131 and #15388

google-cla · 2021-05-19T05:28:29Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

silvervest · 2021-05-19T06:18:16Z

The contents of this PR were approved, and all author CLAs have been signed a couple of times now in the other linked PRs.
@joshdholtz Is there anything (other than time and my messing up the previous PR) blocking this from being approved and merged? Please and thank you!

joshdholtz

@silvervest Hello! Do you mind appending the changelog to the top of the file with a summary of the changes here? 😊 After that we can get this merged 💪

google-cla · 2021-05-19T08:59:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

silvervest · 2021-05-19T09:01:27Z

Done and done @joshdholtz, cheers!

joshdholtz · 2021-05-19T09:02:34Z

@silvervest Thank you! Will smash the merge button when tests are done running 💪

joshdholtz

This looks good to me! Thank you so much for reviving this PR (again (again)) ❤️ Really appreciate it!

silvervest · 2021-05-19T09:04:02Z

Great! And just for the official merge record, want to provide thanks to @kapfab for actually authoring this change in the first place 🥂

kapfab · 2021-05-19T09:24:50Z

You’re welcome. I just hope this PR will finally find its way to a release.

And in case this is needed, I’ll consent to the CLA.

kapfab · 2021-05-19T09:24:56Z

@googlebot I consent.

google-cla · 2021-05-19T09:25:02Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

kapfab · 2021-05-19T10:05:59Z

Just had a quick look at the changes.

I think lines 801-808 (replacement of App ID and Team ID) could be removed, as lines 848-858 do the same on the whole entitlements file (which indeed seems a better option as other entitlements values could reference the App ID or Team ID):

fastlane/sigh/lib/assets/resign.sh

Lines 801 to 808 in abe50be

    
           if [[ "$ID_TYPE" == "APP_ID" ]]; then 
        
               # Replace old value with new value in patched entitlements 
        
               log "Replacing old app ID '$OLD_APP_ID' with new app ID '$NEW_APP_ID'" 
        
               ENTITLEMENTS_VALUE=$(echo "$ENTITLEMENTS_VALUE" | sed "s/$OLD_APP_ID/$NEW_APP_ID/g") 
        
           elif [[ "$ID_TYPE" == "TEAM_ID" ]]; then 
        
               # Replace old team identifier with new value 
        
               log "Replacing old team ID '$OLD_TEAM_ID' with new team ID '$NEW_TEAM_ID'" 
        
               ENTITLEMENTS_VALUE=$(echo "$ENTITLEMENTS_VALUE" | sed "s/$OLD_TEAM_ID/$NEW_TEAM_ID/g")

fastlane/sigh/lib/assets/resign.sh

Lines 848 to 858 in abe50be

    
           if [[ "$ID_TYPE" == "APP_ID" ]]; then 
        
               # Replace old value with new value in patched entitlements 
        
               log "Replacing old app identifier prefix '$OLD_APP_ID' with new value '$NEW_APP_ID'" 
        
               /usr/bin/sed -i .bak "s/$OLD_APP_ID/$NEW_APP_ID/g" "$PATCHED_ENTITLEMENTS" 
        
           elif [[ "$ID_TYPE" == "TEAM_ID" ]]; then 
        
               # Replace old team identifier with new value 
        
               log "Replacing old team ID '$OLD_TEAM_ID' with new team ID: '$NEW_TEAM_ID'" 
        
               /usr/bin/sed -i .bak "s/$OLD_TEAM_ID/$NEW_TEAM_ID/g" "$PATCHED_ENTITLEMENTS" 
        
           else 
        
               continue 
        
           fi

Also, AFAIR, the tr '\n' '\r’ | … | tr '\r' '\n’ (now removed on line 791) was meant to actually remove the entitlements header (which it does not without it). This does not prevent the whole thing to work as plutil handles that gracefully, but the log will dump the headers for each key.

fastlane/sigh/lib/assets/resign.sh

Line 791 in abe50be

    
           ENTITLEMENTS_VALUE="$(PlistBuddy -x -c "Print $KEY" "$APP_ENTITLEMENTS" 2>/dev/null | /usr/bin/sed -e 's,.*<plist[^>]*>\(.*\)</plist>,\1,g')"

kapfab · 2021-05-19T11:09:58Z

@silvervest, did you voluntarily rollback you previous commit (dd77fef)?

Also, is there a reason why if [[ "$certificate_name" =~ "Distribution:" ]]; then changed to this?

fastlane/sigh/lib/assets/resign.sh

Line 827 in abe50be

if [[ "$CERTIFICATE" =~ "Distribution:" ]]; then

I’m a bit worried on this specific change…

silvervest · 2021-05-19T11:28:34Z

@silvervest, did you voluntarily rollback you previous commit (dd77fef)?

Hmm! Not voluntarily. I have no idea how that happened, actually.

Also, is there a reason why if [[ "$certificate_name" =~ "Distribution:" ]]; then changed to this?

fastlane/sigh/lib/assets/resign.sh

Line 827 in abe50be

if [[ "$CERTIFICATE" =~ "Distribution:" ]]; then

That appears to have been a typo from the very first merge conflicts I "fixed" in your original PR. Likely as the change to $certificate_name happened between your original commit and my involvement.
b25f9ce#diff-95cccef747680a2777fd80b592db939260ffbd33ad1e31314635b8b9b01fe933L770-R803

I'll resolve these ASAP and submit another PR. Thanks for catching those @kapfab!

silvervest · 2021-05-19T11:33:55Z

@silvervest, did you voluntarily rollback you previous commit (dd77fef)?

Actually @kapfab I'm looking at this again and I can't see this commit reverted? The file is up to date with this change in master?
https://github.com/fastlane/fastlane/blob/master/sigh/lib/assets/resign.sh#L541

kapfab · 2021-05-19T11:49:15Z

@silvervest, did you voluntarily rollback you previous commit (dd77fef)?

Actually @kapfab I'm looking at this again and I can't see this commit reverted? The file is up to date with this change in master?
https://github.com/fastlane/fastlane/blob/master/sigh/lib/assets/resign.sh#L541

My mistake, I probably saw that on silvervest@caf995e when I was looking for the origin of the certificate-related change.

silvervest · 2021-05-19T11:54:02Z

Also, AFAIR, the tr '\n' '\r’ | … | tr '\r' '\n’ (now removed on line 791) was meant to actually remove the entitlements header (which it does not without it). This does not prevent the whole thing to work as plutil handles that gracefully, but the log will dump the headers for each key.

fastlane/sigh/lib/assets/resign.sh

Line 791 in abe50be

ENTITLEMENTS_VALUE="$(PlistBuddy -x -c "Print $KEY" "$APP_ENTITLEMENTS" 2>/dev/null | /usr/bin/sed -e 's,.*<plist[^>]*>$.*$</plist>,\1,g')"

I am really confused by this. I didn't remove this change, and if you look at the .patch of this pull request, that change is definitely in there, but it wasn't part of the merged changes, nor does it show on the files changed tab. I am at a loss...

kapfab · 2021-05-19T12:15:13Z

Also, AFAIR, the tr '\n' '\r’ | … | tr '\r' '\n’ (now removed on line 791) was meant to actually remove the entitlements header (which it does not without it). This does not prevent the whole thing to work as plutil handles that gracefully, but the log will dump the headers for each key.

fastlane/sigh/lib/assets/resign.sh

Line 791 in abe50be

ENTITLEMENTS_VALUE="$(PlistBuddy -x -c "Print $KEY" "$APP_ENTITLEMENTS" 2>/dev/null | /usr/bin/sed -e 's,.*<plist[^>]*>$.*$</plist>,\1,g')"

I am really confused by this. I didn't remove this change, and if you look at the .patch of this pull request, that change is definitely in there, but it wasn't part of the merged changes, nor does it show on the files changed tab. I am at a loss...

It was part of my initial commit that never reached the master branch (until today!), it probably got lost by someone else months ago during one of the different merges that occurred since. Not a big deal, that might be fixed in a future PR.

fastlane-bot · 2021-05-19T19:31:35Z

Hey @silvervest 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

fastlane-bot · 2021-05-23T19:31:14Z

Congratulations! 🎉 This was released as part of fastlane 2.184.0 🚀

csc-EricWu · 2021-06-03T13:14:59Z

Hi @silvervest , I updated to the latest Fastlane, but this entitlement is invalid for me. Can you have a look?

com.apple.developer.icloud-container-environment

csc-EricWu · 2021-06-03T13:29:10Z

WARNING: Changing team while resigning
WARNING: Using these entitlements from the provisioning profile instead of the existing app:
WARNING: App Groups, Merchant IDs (Apple Pay In-App Payments), iCloud Containers, Pass Type IDs (Wallet)
WARNING: If these capabilities are enabled, make sure AppID and provisioning profile are properly configured
WARNING: Resigned app will allow all pass types from the new team, even if old app only allowed a restricted list
sed: 1: "s/<?xml version="1.0" e ...": unterminated substitute pattern
_floatsignTemp/Payload/Fuse.app: replacing existing signature
_floatsignTemp/patchedEntitlements: Could not modify plist, error: Failed to parse value with type -xml

kapfab · 2021-06-03T13:39:24Z

Hi @silvervest , I updated to the latest Fastlane, but this entitlement is invalid for me. Can you have a look?

com.apple.developer.icloud-container-environment

Which value did you expect and which one did you get?

csc-EricWu · 2021-06-03T13:41:22Z

This is the bag that was made through Xcode.

After Fastlane redesign, it disappeared.

csc-EricWu · 2021-06-03T13:43:00Z

I got an error message

kapfab · 2021-06-03T14:53:35Z

After Fastlane redesign, it disappeared.

The only reason I can see for such a behaviour would be that your app did not include this entitlement but your provisioning profile did, which is quite unusual.
Can you unzip your old IPA and run the following command?

codesign -d --verbose --entitlements :- /path/to/unzipped/ipa/Payload/YourAppName.app

csc-EricWu · 2021-06-03T22:40:26Z

Yes, I tried to try again command, no problem.

csc-EricWu · 2021-06-03T23:16:32Z

@kapfab

fastlane/sigh/lib/assets/resign.sh

Line 833 in abe50be

    
           ENTITLEMENTS_VALUE=$(echo "$ENTITLEMENTS_VALUE" | sed "s/$OLD_ICLOUD_ENV/$NEW_ICLOUD_ENV/g")

In this line, I printed it out empty, adjusted the code here locally, and everything was OK

ENTITLEMENTS_VALUE=$NEW_ICLOUD_ENV

kapfab · 2021-06-04T07:44:14Z

I still do not understand how you original app ended up in this state.
It would be useful to know if this is an isolated case or if a recent change in Xcode broke the whole thing.

csc-EricWu · 2021-06-04T08:05:45Z

You need to try it once, I've found the cause of the problem

ENTITLEMENTS_VALUE=$NEW_ICLOUD_ENV

It works for me.

* Fix the way app entitlements are extracted (see fastlane#18713) * Homogenise sed invocations (following fastlane#17075)

kapfab · 2021-06-04T19:08:09Z

After some investigation, your issue is related to the way entitlement values are extracted (what the lost tr '\n' '\r' | … | tr '\r' '\n' discussed above was meant to fix).

Your workaround might work (even though you feed the — quite permissive — plutil with Production instead of the <string>Production</string> XML element it expects), but this issue will affect other future entitlements whose APP_ID/TEAM_ID/ICLOUD_ENV fragment is to be patched.

I will open a new PR with a less specific fix.

Could you try to resign your app with this change (kapfab@8e98846) to confirm this solves your issue?

csc-EricWu · 2021-06-04T19:55:22Z

After my test, the problem was solved perfectly. And I deliver to Appstore connect. thank you.

gem "fastlane", :git => "https://github.com/kapfab/fastlane", :branch => "resign_app_entitlements_fixes"

* Fix the way app entitlements are extracted (see #18713) * Homogenise sed invocations (following #17075)

kapfab and others added 3 commits September 27, 2019 10:16

Fix entitlements merging when changing team while resigning

b25f9ce

Merge branch 'master' into fix-entitlements-merging

caf995e

Merge branch 'fastlane:master' into fix-entitlements-merging

77dd0cb

google-cla bot added the cla: no label May 19, 2021

silvervest mentioned this pull request May 19, 2021

[sigh] Fix entitlements merging when changing team while resigning #15388

Closed

4 tasks

joshdholtz changed the title ~~Fix entitlements merging~~ [resign] fix entitlements merging when changing team while resigning May 19, 2021

joshdholtz requested changes May 19, 2021

View reviewed changes

update resign.sh changelog

9c3af08

joshdholtz approved these changes May 19, 2021

View reviewed changes

joshdholtz merged commit abe50be into fastlane:master May 19, 2021

silvervest mentioned this pull request May 19, 2021

[resign] fix incorrectly typoed variable name #18715

Merged

4 tasks

fastlane-bot added the status: included-in-next-release label May 19, 2021

joshdholtz mentioned this pull request May 22, 2021

Version bump to 2.184.0 #18733

Merged

fastlane-bot removed the status: included-in-next-release label May 23, 2021

fastlane-bot added the status: released label May 23, 2021

kapfab added a commit to kapfab/fastlane that referenced this pull request Jun 4, 2021

[resign] fix app entitlements extraction

8e98846

* Fix the way app entitlements are extracted (see fastlane#18713) * Homogenise sed invocations (following fastlane#17075)

kapfab mentioned this pull request Jun 4, 2021

[resign] fix app entitlements extraction #18804

Merged

4 tasks

joshdholtz pushed a commit that referenced this pull request Jun 5, 2021

[resign] fix app entitlements extraction (#18804)

9e27dee

* Fix the way app entitlements are extracted (see #18713) * Homogenise sed invocations (following #17075)

fastlane locked and limited conversation to collaborators Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[resign] fix entitlements merging when changing team while resigning #18713

[resign] fix entitlements merging when changing team while resigning #18713

silvervest commented May 19, 2021

google-cla bot commented May 19, 2021

silvervest commented May 19, 2021

joshdholtz left a comment

google-cla bot commented May 19, 2021

silvervest commented May 19, 2021

joshdholtz commented May 19, 2021

joshdholtz left a comment

silvervest commented May 19, 2021

kapfab commented May 19, 2021

kapfab commented May 19, 2021

google-cla bot commented May 19, 2021

kapfab commented May 19, 2021 •

edited

kapfab commented May 19, 2021 •

edited

silvervest commented May 19, 2021 •

edited

silvervest commented May 19, 2021

kapfab commented May 19, 2021

silvervest commented May 19, 2021 •

edited

kapfab commented May 19, 2021

fastlane-bot commented May 19, 2021

fastlane-bot commented May 23, 2021

csc-EricWu commented Jun 3, 2021 •

edited

csc-EricWu commented Jun 3, 2021

kapfab commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

kapfab commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021 •

edited

kapfab commented Jun 4, 2021

csc-EricWu commented Jun 4, 2021

kapfab commented Jun 4, 2021

csc-EricWu commented Jun 4, 2021

[resign] fix entitlements merging when changing team while resigning #18713

[resign] fix entitlements merging when changing team while resigning #18713

Conversation

silvervest commented May 19, 2021

Checklist

Motivation and Context

Description

Testing Steps

google-cla bot commented May 19, 2021

silvervest commented May 19, 2021

joshdholtz left a comment

Choose a reason for hiding this comment

google-cla bot commented May 19, 2021

silvervest commented May 19, 2021

joshdholtz commented May 19, 2021

joshdholtz left a comment

Choose a reason for hiding this comment

silvervest commented May 19, 2021

kapfab commented May 19, 2021

kapfab commented May 19, 2021

google-cla bot commented May 19, 2021

kapfab commented May 19, 2021 • edited

kapfab commented May 19, 2021 • edited

silvervest commented May 19, 2021 • edited

silvervest commented May 19, 2021

kapfab commented May 19, 2021

silvervest commented May 19, 2021 • edited

kapfab commented May 19, 2021

fastlane-bot commented May 19, 2021

fastlane-bot commented May 23, 2021

csc-EricWu commented Jun 3, 2021 • edited

csc-EricWu commented Jun 3, 2021

kapfab commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

kapfab commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021

csc-EricWu commented Jun 3, 2021 • edited

kapfab commented Jun 4, 2021

csc-EricWu commented Jun 4, 2021

kapfab commented Jun 4, 2021

csc-EricWu commented Jun 4, 2021

kapfab commented May 19, 2021 •

edited

kapfab commented May 19, 2021 •

edited

silvervest commented May 19, 2021 •

edited

silvervest commented May 19, 2021 •

edited

csc-EricWu commented Jun 3, 2021 •

edited

csc-EricWu commented Jun 3, 2021 •

edited