Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add trackVerifiedToken() #270

Merged
merged 7 commits into from
Aug 22, 2023
Merged

add trackVerifiedToken() #270

merged 7 commits into from
Aug 22, 2023

Conversation

nickpatrick
Copy link
Contributor

No description provided.

RadarSDK/RadarUtils.m Outdated Show resolved Hide resolved
Comment on lines +339 to +347
if (encrypted) {
if (!tokenObj) {
return completionHandler(status, nil, nil, nil, nil, nil, nil);
}

NSString *token = (NSString *)tokenObj;

return completionHandler(status, nil, nil, nil, nil, nil, token);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codepath is completely forgoing updating the event & location update handlers, trips, & place updating.

As such, trackVerified and trackVerifiedEncrypted behave significantly differently -- wonder how we should make it clear to users the difference in behavior and effects (could see myself getting confused by this).

Copy link
Contributor Author

@nickpatrick nickpatrick Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will mention on /documentation. Most people using this aren't going to be using startTracking(), so shouldn't cause unexpected behavior in practice

Copy link
Contributor

@jsani-radar jsani-radar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good overall - few small things

Radar.setDelegate(self)

Radar.trackVerified { status, location, events, user in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: print result?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a weird issue with this:

        
        Radar.trackVerifiedEncrypted{ ( status, token ) in
            NSLog("Got token:")
            NSLog(token!)
        }

The token printed was truncated to 1,024 characters, but the token returned from the server was 7,000+ characters.

While running the debugger, and clicking Copy, would sometimes copy the truncated value, but not always:

image

I had to run print (void)CFShow(token) in the debugging console to show the entire token to confirm it was being captured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a similar issue testing on Android where logcat lines seem to be truncated to 4000 chars, but my token was something like 4300. I just printed it in two parts. In any case, I reverted this. We can revisit inclusion in sample app when we revisit sample app code more broadly


Receives the request status and, if successful, a JSON Web Token (JWT) containing an array of the events generated and the user. Decrypt the JWT server-side using your private key.

@see https://radar.com/documentation/sdk/ios
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be pointing to the iOS docs or fraud docs? comments above the callback and method are inconsistent at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, will update

@@ -219,7 +219,7 @@ + (void)trackOnceWithLocation:(CLLocation *)location completionHandler:(RadarTra
replayed:NO
beacons:nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a use case where a customer would want the payload encrypted through the vanilla trackOnce call and not necessarily need SSL pinning? wondering if we should add an encrypted option here too

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is, I think if they want the encrypted payload they have to do the full trackVerified experience.

@@ -219,7 +219,7 @@ + (void)trackOnceWithLocation:(CLLocation *)location completionHandler:(RadarTra
replayed:NO
beacons:nil
completionHandler:^(RadarStatus status, NSDictionary *_Nullable res, NSArray<RadarEvent *> *_Nullable events, RadarUser *_Nullable user,
NSArray<RadarGeofence *> *_Nullable nearbyGeofences, RadarConfig *_Nullable config) {
NSArray<RadarGeofence *> *_Nullable nearbyGeofences, RadarConfig *_Nullable config, NSString *_Nullable token) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we call out anywhere that the rest of the objects in this callback will be nil if token is present?

Copy link
Contributor Author

@nickpatrick nickpatrick Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback is only ever used internally, not worried about it

Comment on lines 178 to 189
NSString *testPath = @"/private/JailbreakTest.txt";
NSData *data = [@"This is a test." dataUsingEncoding:NSUTF8StringEncoding];

NSError *error;
[data writeToFile:testPath options:NSDataWritingAtomic error:&error];

// If no error occurred, the device is likely jailbroken
if (!error) {
NSLog(@"failed check 2");
[[NSFileManager defaultManager] removeItemAtPath:testPath error:nil];
return YES;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first test makes sense to me.

For this test here, I'd love to read up on why this test works — is there a SO or other resource you can point me to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually work, reverting

@nickpatrick nickpatrick changed the title add trackVerifiedEncrypted() add trackVerifiedToken() Aug 21, 2023
@nickpatrick nickpatrick marked this pull request as ready for review August 21, 2023 22:13
@@ -86,6 +87,7 @@ typedef void (^_Nonnull RadarSyncLogsAPICompletionHandler)(RadarStatus status);
attestationString:(NSString *_Nullable)attestationString
keyId:(NSString *_Nullable)keyId
attestationError:(NSString *_Nullable)attestationError
encrypted:(BOOL)encrypted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was going to suggest we think about changing this to verified given discussion in slack & recent changes, but of course that's already used.. 🤔

Copy link
Contributor

@lmeier lmeier Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the JWT docs, I think encoded instead of encrypted would perhaps be most accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking we can change param or endpoint name or switch to a project setting later on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

token param or POST /track/token endpoint are both options, too

@nickpatrick nickpatrick merged commit f9c70e4 into master Aug 22, 2023
1 check passed
@nickpatrick nickpatrick deleted the track-verified-encrypted branch August 22, 2023 10:01
ShiCheng-Lu pushed a commit that referenced this pull request Jun 13, 2024
* First part of offline replay

* Fix kotlin errors

* Fix params for replays

* Extend timeout

* Fix unallocated val bug

* Fix nowMS bug

* Fix connectTimeout numbers

* Bump version to 3.5.12-beta.2 to cut a prerelease

* Bump nexus client timeout

* Fix bugs in buffer logic

* bump version to 3.5.11-beta.6
ShiCheng-Lu pushed a commit that referenced this pull request Jun 13, 2024
* publish SNAPSHOT releases from pre-releases not from pushing to develop (#259)

* don't publish pre-releases to maven as a release

* publish SNAPSHOTs from pre-releases not from pushing to develop

* MOB-57: update the README with more detail on example app (#260)

Co-authored-by: Jason R Tibbetts <[email protected]>
Co-authored-by: Nick Patrick <[email protected]>
Co-authored-by: Tim Julien <[email protected]>

* Surface center, radius, and location for geofences & places (#267)

* Surface center, radius, and location for geofences & places

* rely on geofence's geometry for center and radius, add test

* RAD-5747 Anonymous config call in `trackOnce` (#266)

* add config call for anonymous before track

* add boolean for track usage

* usage enum not bool

* resume usage

* move config track usage call later in the call stack

* move config track usage call later in the call stack

* use anon val

* bump version

* 3.5.11 version bump

---------

Co-authored-by: Jason R Tibbetts <[email protected]>
Co-authored-by: Nick Patrick <[email protected]>
Co-authored-by: Tim Julien <[email protected]>

* Include approachingThreshold in tripOptions (#261)

* Include approachingThreshold in trip options helper functions

* Only include in dictionary if > 0, add test

* Offline replay (#270)

* First part of offline replay

* Fix kotlin errors

* Fix params for replays

* Extend timeout

* Fix unallocated val bug

* Fix nowMS bug

* Fix connectTimeout numbers

* Bump version to 3.5.12-beta.2 to cut a prerelease

* Bump nexus client timeout

* Fix bugs in buffer logic

* bump version to 3.5.11-beta.6

* dont fire event listeners for custom events (#263)

* publish beta's the same way we publish final releases (#273)

* publish beta's the same way we publish final releases

* Delete radar-snapshot-action.yml

* Update build.gradle

* Always clear last location if successful track (#272)

* Always clear last location if succesful track

* Always use System.currentTimeMillis()

* Always clear ReplayBuffer upon successful request

---------

Co-authored-by: Tim Julien <[email protected]>

* version bump

* set replay = NONE for continuous

* not beta anymore

---------

Co-authored-by: Travis Derouin <[email protected]>
Co-authored-by: Jason R Tibbetts <[email protected]>
Co-authored-by: Nick Patrick <[email protected]>
Co-authored-by: Liam Meier <[email protected]>
Co-authored-by: David Goodfellow <[email protected]>
Co-authored-by: jsani-radar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants