OneBusAway / onebusaway-iphone

OBA development has moved!
https://github.com/OneBusAway/OBAKit
Apache License 2.0
219 stars 117 forks source link

Implement TestFlight (& FlightPath) SDK #21

Closed bbodenmiller closed 11 years ago

bbodenmiller commented 11 years ago

Assuming we implement TestFlight we should implement the TestFlight SDK as outlined at https://testflightapp.com/sdk/ios/doc/. This will provide better insights into crashes, etc. during testing (and production as FlightPath comes online).

neophit commented 11 years ago

I think we should try to protect the integrity of the test data by encrypting the TestFlight API tokens in the public repository and limit API token access to trusted developers on the TestFlight team. Anonymous developers cloning the repository on Github will not benefit from TestFlight integration since they do not have access to the TestFlight Dashboard.

When implementing TestFlight, I recommend creating a header file with dummy API tokens to commit in the public repository. The real API tokens should be encrypted and only used by the continuous integration server when deploying beta builds. To do this with Travis CI, the API keys should be encrypted in .travis.yml along with a before_script that inserts the real API keys into the header file before compiling.

TestFlight team developers can replace the dummy API tokens in their working copy when testing TestFlight integration, but should be extra careful not to commit and push the tokens to the public repo.

I'd be happy to help with this implementation or provide documentation for Travis CI integration.

bbodenmiller commented 11 years ago

@neophit if you could create some documentation around how you think we should handle to SDK token that would be great. Sadly I'm not sure we can do it via Travis CI pushing to TestFlight for now given the concerns with using the enterprise license in the app. For the time being it is probably best for us to use MacBuildServer but that will result in the SDK token having to be in the repo since that is the only way I have to build (Windows & Linux only here).

bbodenmiller commented 11 years ago

Any beta's including this should include:

By installing this beta you are agreeing to the TestFlight Privacy Policy: https://www.testflightapp.com/privacy/

Any public releases with FlightPath should result in the OBA privacy policy being updated to say that you also agree to the FlightPath Privacy Policy – Users: https://www.testflightapp.com/privacy/

neophit commented 11 years ago

My main concern is that the API tokens are used by the TestFlight Upload API to distribute builds to testers. If the tokens are committed to the public repository then it becomes trivial for someone to distribute a build of OBA with malicious code to the TestFlight members.

StackExchange has some good ideas for protecting sensitive data in a public repository.

bbodenmiller commented 11 years ago

Just to be clear TestFlight has an Upload, App, and Team token.

Useful feedback module: http://help.testflightapp.com/customer/portal/articles/829567-how-do-i-implement-the-feedback-module-

bbodenmiller commented 11 years ago

@ksslng @neophit I've attempted adding TestFlight SDK manually (don't have a Mac) based on exact code from others who have implemented it but am getting errors such as [PBXBuildFile setGroup:]: unrecognized selector sent to instance and more. Code is at https://github.com/bbodenmiller/onebusaway-iphone/tree/testflight-sdk, CI at https://travis-ci.org/bbodenmiller/onebusaway-iphone/builds. Can you help debug? Thanks!

neophit commented 11 years ago

I was able to implement the TestFlight SDK v2.0.0 in the project using Xcode. We can see my device starting and ending sessions in the TestFlight SDK Debugger. screen shot 2013-08-15 at 20 55 11 Xcode typically manages changes to files inside the .xcodeproj folder. Unfortunately, I can't help manually modify project settings outside of Xcode.

bbodenmiller commented 11 years ago

Could you possible just commit your OBA code with the TestFlight SDK working? I can probably figure it out from there.

neophit commented 11 years ago

Here you go, @bbodenmiller. Sorry for not commiting last night; was having issues with my github credentials.

bbodenmiller commented 11 years ago

No worries, thanks.

bbodenmiller commented 11 years ago

@neophit did the results show up in the sdk debugger near instantaneously? If so any thoughts on why https://github.com/bbodenmiller/onebusaway-iphone/tree/testflight2 isn't working?

neophit commented 11 years ago

The results usually appear within a minute; sometimes nearly instantaneous. I was able to download your branch, run it on the simulator and on my device with results in the SDK debugger. Do you see any errors? Check the console log for these lines after launching the app:

2013-08-17 18:33:34:638 OneBusAway[29139:3079] TestFlight: Crash Handlers are installed
2013-08-17 18:33:36:155 OneBusAway[29139:18435] TestFlight: Started Session
2013-08-17 18:33:37:184 OneBusAway[29139:4867] TestFlight: App Token is recognized

Apple deprecated the uniqueIdentifier property and will reject apps using it when submitted to the App Store. Remember to remove this line before submission:

[TestFlight setDeviceIdentifier:[[UIDevice currentDevice] uniqueIdentifier]];
aaronbrethorst commented 11 years ago

The udid call should be replaced with IDFV: http://developer.apple.com/library/ios/documentation/uikit/reference/UIDevice_Class/Reference/UIDevice.html#//apple_ref/occ/instp/UIDevice/identifierForVendor

Sent from my iPhone

On Aug 17, 2013, at 6:58 PM, Jon notifications@github.com wrote:

The results usually appear within a minute; sometimes nearly instantaneous. I was able to download your branch, run it on the simulator and on my device with results in the SDK debugger. Do you see any errors? Check the console log for these lines after launching the app:

2013-08-17 18:33:34:638 OneBusAway[29139:3079] TestFlight: Crash Handlers are installed 2013-08-17 18:33:36:155 OneBusAway[29139:18435] TestFlight: Started Session 2013-08-17 18:33:37:184 OneBusAway[29139:4867] TestFlight: App Token is recognized Apple deprecated the uniqueIdentifier property and will reject apps using it when submitted to the App Store. Remember to remove this line before submission:

[TestFlight setDeviceIdentifier:[[UIDevice currentDevice] uniqueIdentifier]]; — Reply to this email directly or view it on GitHub.

neophit commented 11 years ago

@aaronbrethorst that is the correct API to use for the deprecated uniqueIdentifier property, however TestFlight SDK session data will appear as anonymous on TestFlight since they only collect the deprecated UDID during test device registration.

aaronbrethorst commented 11 years ago

That's a bummer. TestFlight mentions:

In iOS 7, uniqueIdentifier no longer returns the device's UDID, so iOS 7 users will show up anonymously on TestFlight even if you use setDeviceIdentifier:. In App Updates will also not work.

https://testflightapp.com/sdk/ios/known_issues/

On Aug 17, 2013, at 8:44 PM, Jon notifications@github.com wrote:

@aaronbrethorst that is the correct API to use for the deprecated uniqueIdentifier property, however TestFlight SDK session data will appear as anonymous on TestFlight since they only collect the deprecated UDID during test device registration.

— Reply to this email directly or view it on GitHub.

bbodenmiller commented 11 years ago

Any idea how I can check the console log without Xcode?

neophit commented 11 years ago

Use the iPhone Configuration Utility to get console output without Xcode. Or use one of the free console apps on your device.

bbodenmiller commented 11 years ago

@neophit can you verify these recommended Xcode project settings are set in our develop branch? Thanks.

neophit commented 11 years ago

@bbodenmiller I verified the recommended settings are set in the develop branch and created PR #139 to change "Strip Debug Symbols During Copy" from Yes to No for all project schemes.

bbodenmiller commented 11 years ago

Thanks!

neophit commented 11 years ago

Using a preprocessor macro in the project settings, we can automatically exclude the deprecated uniqueIdentifier from the AppStoreRelease scheme when submitting to the App Store by wrapping the code in a preprocessor directive like this:

#ifdef DEBUG
[TestFlight setDeviceIdentifier:[[UIDevice currentDevice] uniqueIdentifier]];
#endif

I commited another change to PR #139 with the preprocessor macro "DEBUG" for the Debug scheme in project settings.

bbodenmiller commented 11 years ago

Is this just applied based on whether you declare it as a debug build, App Store build, etc.?

neophit commented 11 years ago

It is applied based on the specified scheme when building. For instance, in travis.yml, the argument "-sceme Debug" tells xctool to build the project using the scheme named Debug. Preprocess macros are defined per scheme so if the AppStoreRelease scheme is specified, then the "DEBUG" macro is not defined and any code wrapped inside the #ifdef ... #endif will be ignored by the compiler.

This method is recommended on TestFlight's blog, "We recommend that when you choose to set +setDeviceIdentifier:(NSString)deviceIdentifier to the current device’s unique identifier that you wrap the method in a preprocessor directive that only enables the call when you are building for ad-hoc release."

bbodenmiller commented 11 years ago

Sounds like a good thing to add. Can you submit the PR?

neophit commented 11 years ago

The change was submitted to PR #139 which has been merged into develop, but the setDeviceIdentifier call was not wrapped in the preprocessor macro. I created PR #140 to wrap the setDeviceIdentifier call.

bbodenmiller commented 11 years ago

@neophit For some reason the build (via Travis CI) and install (via the jailbreak install) method I'm using still isn't loading TestFlight. Obviously this isn't a big deal for most since I'm just going around the standard processes. Any chance you could build via the command line we have setup in Travis CI and then install to verify it is my install method that is broken? Or maybe just double check against the system console log while running a “build and archive” through Xcode.

bbodenmiller commented 11 years ago

As per https://github.com/OneBusAway/onebusaway-iphone/issues/129#issuecomment-23612995 appears my install has been reporting to TestFlight. For some reason the system console app hasn't been printing TestFlight: Started Session and it still isn't showing the debugger. How odd.

bbodenmiller commented 11 years ago

Is anyone else getting a console messages along the lines of geod <Warning> Can't get bundle identifier for process 14473?

neophit commented 11 years ago

TestFlight support responded to my inquiry regarding use of the deprecated uniqueIdentifier on iOS 7:

TestFlight will still report data from iOS devices however it will show up as anonymous user.
Our dev team is working on this and we hope to have addressed concerns when it is released live.
bbodenmiller commented 11 years ago

Where does this uniqueIdentifier even show up in TestFlight? In my crash data it didn't link it to my device.

neophit commented 11 years ago

TestFlight uses the uniqueIdentifier to match SDK sessions, checkpoints, feedback, and crash reports to specific TestFlight users. You can your devices' UDID registered with TestFlight on your account devices page. This should match your UDID in iTunes. I'll do some debugging this morning to see if my device is sending the correct UDID.

neophit commented 11 years ago

In the Xcode project settings, the 'Debug' scheme is using the 'AppStoreDistribution' build configuration when Archiving (like we do in travis.yml). This means the DEBUG preprocessor macro is undefined and -setDeviceIdentifier: is never called. This can be fixed by changing the build configuration from 'AppStoreDistribution' to 'AdHocDistribution' and verifying the provisioning profile specified for the 'AdHocDistribution' build configuration is valid for distribution on TestFlight. Also, the DEBUG preprocessor macro will need to be defined for the 'AdHocDistribution' build configuration.

I created a build with these changes on my machine, but used my developer account's provisioning profile which will not install on any TestFlight users' devices. You can see the session data and crash reports are correctly matched to my TestFlight user. The crashes were caused by a line I added to throw an exception for testing the crash reporting.

On a privacy note, TestFlight collects some console log output which includes anything from NSLog such as URL requests containing the user's GPS location:

2013-09-06 14:00:46 -[OBADataSourceConfig constructURL:withArgs:includeArgs:] [Line 60] url=http://api.pugetsound.onebusaway.org/api/where/stops-for-location.json?key=org.onebusaway.iphone&app_uid=9365825C-6668-49CD-861C-572DA1ECCB9C&app_ver=2.1b1&lat=47.654469&lon=-122.348342&latSpan=0.007957&lonSpan=0.010300&version=2
bbodenmiller commented 11 years ago

Note @neophit made the discussed changes above in PR https://github.com/OneBusAway/onebusaway-iphone/pull/150.

caitbonnar commented 11 years ago

Thanks, @neophit! Do I need to submit a PR with the enterprise distribution profile in the build config, or will just having it selected as the general "AdHocDistribution" work?

caitbonnar commented 11 years ago

Oh, and thanks for the note on the GPS location... hmm. So could this potentially link a user to their GPS location? If it is only for beta-testing, this would be less troubling, but we should definitely make a note of it. If also in App store distribution, we'd need to get it out.

bbodenmiller commented 11 years ago

@caitbonnar I think it'd be best for you to fix up as you submit to TestFlight and then commit back any setting changes.

As for the GPS stuff it should only be traceable when beta testing.

bbodenmiller commented 11 years ago

@neophit what code did you use to force a crash?

neophit commented 11 years ago

@caitbonnar since Travis is building an Archive without code signing, the enterprise provisioning profile does not need to be committed. You should be able to use the enterprise distribution profile when code signing prior to uploading to TestFlight.

@bbodenmiller I used this line of code to force a crash:

[self performSelector:@selector(crash) withObject:nil afterDelay:0];
bbodenmiller commented 11 years ago

I agree with @neophit just meant you might want to commit the settings so you don't have to fix them for every release.

caitbonnar commented 11 years ago

@neophit @bbodenmiller Thanks, makes sense.

bbodenmiller commented 11 years ago

@caitbonnar do you remember if you built as an 'AdHocDistribution' build for 2.1b3? I ask because TestFlight and TestFlight support indicate that #ifdef DEBUG did not evaluate as TRUE and thus the wrong App Token is being used as well as recording everything as anonymous. I'm trying to figure out if it was user mistake or if there's some build configuration error we need to track down.

caitbonnar commented 11 years ago

@bbodenmiller It should be. I saved it for Ad Hoc/Enterprise distribution when I saved the .ipa file. But maybe there's some other reason it's saying that-- the Xcode 5 build settings are a bit confusing since they split the code signing identity and provisioning profile selection. As I have it now, code signing should be selected as "automatic" and the AdHocDistribution and AppStoreDistribution provisioning profile has the ad hoc/in-house distribution profile that I made selected. (For some reason I had to select it under AppStoreDistribution as well or it wouldn't let me build.)

Edit:: There's also a "Debug" code signing/provisioning profile selector; currently iOS developer/nothing is selected for that. Don't know if that affects it or not.

bbodenmiller commented 11 years ago

I'll create a PR with some more visual debugging details to help troubleshoot.

caitbonnar commented 11 years ago

@bbodenmiller Okay, sounds good. Was it working for 2.1b2?

bbodenmiller commented 11 years ago

Don't think so, hard to tell exactly.

bbodenmiller commented 11 years ago

Okay PR https://github.com/OneBusAway/onebusaway-iphone/issues/197 should help you figure out the correct build settings.

caitbonnar commented 11 years ago

Closing because I think everything is implemented now. If I'm wrong please reopen.