Certificate wizard: register Macs, report the right slot, check the profile signs this app - #5815
Merged
Merged
Conversation
…rofile signs this app Issues #5773 and #5793, whose reports overlap. Four defects, each independent. **A Mac cannot be registered.** CloudSigningService.registerDevice hardcoded "IOS". The service has always accepted MAC_OS -- only the dialog never asked -- so a wizard that offers MAC_APP_DEVELOPMENT offered a profile type nothing could satisfy: isUsableDevice correctly hides an iPhone from a Mac profile's picker, and no Mac could be put in it. The registration dialog now picks the platform, and the UDID hint follows it, because a Mac is registered by its hardware UUID rather than by an iPhone's 25-character identifier. **The overview announced readiness it did not have.** Both cards read row zero of an unfiltered list, so an account holding a Mac certificate and a development profile was told "Apple distribution certificate: Ready -- dtest11 MAC APP DISTRIBUTION" and "App Store profile: Ready -- dtest11 Development". Neither slot was filled. The wizard was never going to USE either for an iOS build -- certificateTypeSatisfies has always been strict, and that part needed no change -- but the panel is why the reporter concluded it was signing iOS builds with Mac App Store assets, and a readiness panel that lies is worse than none. The selection moved to WizardDecisions, where it can be tested without a display, and the profile card now requires the type AND that Apple has not marked it INVALID. **A downloaded "Apple Development" certificate was offered to the release slot.** afterCertificateDownload tested certificateType against IOS_DEVELOPMENT alone, while every other decision in the class goes through isDevelopmentCertificate, which also counts Apple's generic DEVELOPMENT type. So a manual download of an Apple Development certificate would install into codename1.ios.release.certificate and overwrite the unqualified key with it. **"Sync with Apple" could not sync profiles at all**, because the signing service had no endpoint to sync them with -- the companion BuildCloud change adds POST /profiles/reconcile and explains why. The wizard now calls both halves. A 404/405 from the profile half is tolerated so that a wizard newer than the deployed service still syncs certificates instead of failing outright; any other status is reported. Also here: IOSProvisioningPreflight was applying profileCoversBundleId to every app EXTENSION and never to the app itself, so the app's own profile was checked only for readability, expiry and distribution method. A profile issued for a different App ID passed preflight and failed minutes later on the build server with "Provisioning profile ... doesn't match the entitlements file's values for the application-identifier and keychain-access-groups entitlements" -- both of which are $(AppIdentifierPrefix)$(CFBundleIdentifier), which is to say both of them are that comparison, spelled by Xcode after the upload. Both reporters spent a cloud build each to be told it. What it deliberately does not catch is the same bundle id under a different TEAM prefix: nothing in codenameone_settings.properties states the team, so that half stays a server-side failure, and there is a test asserting the limit rather than leaving it implied. The wizard's surefire skipTests was hardcoded true, which -DskipTests=false could not reach, so its 78 existing tests could not be run here or anywhere. It is now the same opt-in property scripts/settings/common uses, for the same reason, with the same default. Verified locally: 81 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ac2a37ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding on #5815, and correct -- and it is the same defect this PR set out to fix, in a narrower form that the companion BuildCloud change makes more likely rather than less. appStoreProfileForOverview returned the first ACTIVE IOS_APP_STORE profile on the account without looking at its bundle id. Before this PR the card showed profiles.get(0), so any profile at all; after it, any App Store profile on the account. Once "Sync with Apple" imports the whole account rather than only what this wizard created (codenameone/BuildCloud#147), a team that ships more than one app is likely to have another app's App Store profile matched first -- announced "Ready" here, and then refused for this project by the very preflight check added in this same PR. So the selection now matches the bundle id too, and the doc lists all three things that have to line up, since each has been wrong in this one card. A blank or unreadable project identifier skips the filter rather than guessing it, which is the rule IOSProvisioningPreflight follows for the same reason: a card reading "None yet" beside a perfectly good profile is its own kind of wrong. The certificate card is deliberately NOT scoped this way. An Apple Distribution certificate signs anything the team ships; a provisioning profile is issued for one App ID. That asymmetry is noted at the call site so it does not read as an oversight. 82 wizard tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5773 and the client half of #5793. Four independent defects plus a preflight gap.
1. A Mac cannot be registered (#5773)
CloudSigningService.registerDevicehardcoded"IOS". The signing service has always acceptedMAC_OS— only the dialog never asked. So the wizard offered aMAC_APP_DEVELOPMENTprofile type that nothing could satisfy:isUsableDevicecorrectly hides an iPhone from a Mac profile's picker, and no Mac could be registered to put in it. A profile type you can select and never create.The registration dialog now picks the platform, and the UDID hint follows it — a Mac is registered by its hardware UUID, not by an iPhone's 25-character identifier.
The reporter's screenshot is the iOS profile device picker, where hiding Macs is correct. The bug is the missing registration path, not that screen.
2. The overview announced readiness it did not have (#5773)
Both cards read row zero of an unfiltered list:
So an account holding a Mac certificate and a development profile was told "Apple distribution certificate: Ready — dtest11 MAC APP DISTRIBUTION" and "App Store profile: Ready — dtest11 Development". Neither slot was filled.
To be fair to the code: the wizard was never going to use either for an iOS build.
WizardDecisions.certificateTypeSatisfieshas always been strict and needed no change. But that panel is why the reporter concluded it was signing iOS builds with Mac App Store assets, and a readiness panel that reports readiness it does not have is worse than no panel at all.The selection moved to
WizardDecisions, where it is testable without a display. The profile card now requires the type and that Apple has not marked the profile INVALID — "Ready" for a profile the next build cannot sign with is the same false assurance.3. A downloaded "Apple Development" certificate went to the release slot
afterCertificateDownloadtested"IOS_DEVELOPMENT".equals(...)alone, while every other decision in the class goes throughisDevelopmentCertificate, which also counts Apple's genericDEVELOPMENTtype. A manual download of an Apple Development certificate therefore offered to install intocodename1.ios.release.certificate, overwriting the unqualified key with it.4. "Sync with Apple" calls both halves (#5793)
It called the certificate reconcile alone — and there was no profile reconcile to call. Companion PR: codenameone/BuildCloud#147, which explains why the profile list could never come into step with Apple.
A 404/405 from the new route is tolerated, so a wizard newer than the deployed service still syncs certificates instead of failing outright. Any other status is reported.
Also: the preflight never checked the app's own profile
IOSProvisioningPreflightappliesprofileCoversBundleIdto every app extension and never to the app itself, so the app's profile was checked only for readability, expiry and distribution method. A profile issued for a different App ID passed preflight and failed minutes later on the build server with:Both of those entitlements are
$(AppIdentifierPrefix)$(CFBundleIdentifier)— which is to say both of them are that comparison, spelled by Xcode after the upload. Both reporters spent a cloud build each to be told it, and every fact needed was on disk beforehand.It deliberately does not catch the same bundle id under a different team prefix: nothing in
codenameone_settings.propertiesstates the team, so that half stays a server-side failure. There is a test asserting the limit rather than leaving it implied.(Incidentally: #5793's own diagnosis of this error — the multicast entitlement — is wrong. That would name
com.apple.developer.networking.multicast.)Test infrastructure
The wizard's surefire
skipTestswas hardcodedtrue, which-DskipTests=falsecannot reach, so its 78 existing tests could not be run here or anywhere. It is now the same opt-in propertyscripts/settings/commonuses, for the same reason, with the same default:Verification
IOSProvisioningPreflightTest: 45 pass (39 existing + 6 new). A/B probe run — stubbing out the new check makesprofileForAnotherAppIsRefusedfail, so it does bite.codenameone-maven-plugin: 0 findings.Ordering
codenameone/BuildCloud#147 should be deployed first. Nothing here breaks without it — the sync degrades to today's behaviour — but the profile half stays inert until that route exists.
🤖 Generated with Claude Code