From 135243e3830efd3158aba331f71680594bf3ae48 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 14 Aug 2017 14:51:51 -0400 Subject: [PATCH] CR: variable rename, better comments, fix up tests // FREEBIE --- Signal/src/Profiles/OWSProfileManager.h | 2 +- Signal/src/Profiles/OWSProfileManager.m | 56 +++++++++---------- Signal/src/Profiles/ProfileFetcherJob.swift | 7 +-- .../src/Network/API/Requests/TSRequest.h | 5 -- .../TestSupport/Fakes/OWSFakeProfileManager.m | 19 +++++-- 5 files changed, 46 insertions(+), 43 deletions(-) diff --git a/Signal/src/Profiles/OWSProfileManager.h b/Signal/src/Profiles/OWSProfileManager.h index 59fb9c8c3..d460c12b0 100644 --- a/Signal/src/Profiles/OWSProfileManager.h +++ b/Signal/src/Profiles/OWSProfileManager.h @@ -61,7 +61,7 @@ extern NSString *const kNSNotificationName_OtherUsersProfileDidChange; - (void)updateProfileForRecipientId:(NSString *)recipientId profileNameEncrypted:(nullable NSData *)profileNameEncrypted - avatarUrl:(nullable NSString *)avatarUrl; + avatarUrlPath:(nullable NSString *)avatarUrlPath; @end diff --git a/Signal/src/Profiles/OWSProfileManager.m b/Signal/src/Profiles/OWSProfileManager.m index c5272059b..f975be3a0 100644 --- a/Signal/src/Profiles/OWSProfileManager.m +++ b/Signal/src/Profiles/OWSProfileManager.m @@ -30,9 +30,7 @@ NS_ASSUME_NONNULL_BEGIN // These properties may be accessed only from the main thread. @property (nonatomic, nullable) NSString *profileName; -// TODO This isn't really a URL, since it doesn't contain the host. -// rename to "avatarPath" or "avatarKey" -@property (nonatomic, nullable) NSString *avatarUrl; +@property (nonatomic, nullable) NSString *avatarUrlPath; // This filename is relative to OWSProfileManager.profileAvatarsDirPath. @property (nonatomic, nullable) NSString *avatarFileName; @@ -72,13 +70,13 @@ NS_ASSUME_NONNULL_BEGIN - (BOOL)isEqual:(UserProfile *)other { return ([other isKindOfClass:[UserProfile class]] && [self.recipientId isEqualToString:other.recipientId] && - [self.profileName isEqualToString:other.profileName] && [self.avatarUrl isEqualToString:other.avatarUrl] && + [self.profileName isEqualToString:other.profileName] && [self.avatarUrlPath isEqualToString:other.avatarUrlPath] && [self.avatarFileName isEqualToString:other.avatarFileName]); } - (NSUInteger)hash { - return self.recipientId.hash ^ self.profileName.hash ^ self.avatarUrl.hash ^ self.avatarFileName.hash; + return self.recipientId.hash ^ self.profileName.hash ^ self.avatarUrlPath.hash ^ self.avatarFileName.hash; } @end @@ -306,7 +304,7 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; // * Try to update the service. // * Update client state on success. void (^tryToUpdateService)(NSString *_Nullable, NSString *_Nullable) = ^( - NSString *_Nullable avatarUrl, NSString *_Nullable avatarFileName) { + NSString *_Nullable avatarUrlPath, NSString *_Nullable avatarFileName) { [self updateServiceWithProfileName:profileName success:^{ // All reads and writes to user profiles should happen on the main thread. @@ -315,9 +313,9 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; OWSAssert(userProfile); userProfile.profileName = profileName; - // TODO remote avatarUrl changes as result of fetching form - + // TODO remote avatarUrlPath changes as result of fetching form - // we should probably invalidate it at that point, and refresh again when uploading file completes. - userProfile.avatarUrl = avatarUrl; + userProfile.avatarUrlPath = avatarUrlPath; userProfile.avatarFileName = avatarFileName; [self saveUserProfile:userProfile]; @@ -345,19 +343,19 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; // * Upload it to asset service // * Send asset service info to Signal Service if (self.localCachedAvatarImage == avatarImage) { - OWSAssert(userProfile.avatarUrl.length > 0); + OWSAssert(userProfile.avatarUrlPath.length > 0); OWSAssert(userProfile.avatarFileName.length > 0); DDLogVerbose(@"%@ Updating local profile on service with unchanged avatar.", self.tag); // If the avatar hasn't changed, reuse the existing metadata. - tryToUpdateService(userProfile.avatarUrl, userProfile.avatarFileName); + tryToUpdateService(userProfile.avatarUrlPath, userProfile.avatarFileName); } else { DDLogVerbose(@"%@ Updating local profile on service with new avatar.", self.tag); [self writeAvatarToDisk:avatarImage success:^(NSData *data, NSString *fileName) { [self uploadAvatarToService:data - success:^(NSString *avatarUrl) { - tryToUpdateService(avatarUrl, fileName); + success:^(NSString *avatarUrlPath) { + tryToUpdateService(avatarUrlPath, fileName); } failure:^{ failureBlock(); @@ -401,7 +399,7 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; } - (void)uploadAvatarToService:(NSData *)avatarData - success:(void (^)(NSString *avatarUrl))successBlock + success:(void (^)(NSString *avatarUrlPath))successBlock failure:(void (^)())failureBlock { OWSAssert(avatarData.length > 0); @@ -509,15 +507,15 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; // but the approach of getting it from the remote provider seems a more // robust way to ensure we've actually created the resource where we // think we have. - NSString *avatarURL = response.allHeaderFields[@"Location"]; - if (avatarURL.length == 0) { + NSString *avatarUrlPath = response.allHeaderFields[@"Location"]; + if (avatarUrlPath.length == 0) { OWSProdFail(@"profile_manager_error_avatar_upload_no_location_in_response"); failureBlock(); return; } - DDLogVerbose(@"%@ successfully uploaded avatar url: %@", self.tag, avatarURL); - successBlock(avatarURL); + DDLogVerbose(@"%@ successfully uploaded avatar url: %@", self.tag, avatarUrlPath); + successBlock(avatarUrlPath); } failure:^(NSURLSessionDataTask *_Nullable uploadTask, NSError *_Nonnull error) { DDLogVerbose(@"%@ uploading avatar failed with error: %@", self.tag, error); @@ -691,7 +689,7 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; // Clear profile state. userProfile.profileName = nil; - userProfile.avatarUrl = nil; + userProfile.avatarUrlPath = nil; userProfile.avatarFileName = nil; [self saveUserProfile:userProfile]; @@ -738,7 +736,7 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; if (image) { [self.otherUsersProfileAvatarImageCache setObject:image forKey:recipientId]; } - } else if (userProfile.avatarUrl.length > 0) { + } else if (userProfile.avatarUrlPath.length > 0) { [self downloadAvatarForUserProfile:userProfile]; } @@ -750,12 +748,12 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; OWSAssert([NSThread isMainThread]); OWSAssert(userProfile); - if (userProfile.avatarUrl.length < 1) { - OWSFail(@"%@ Malformed avatar URL: %@", self.tag, userProfile.avatarUrl); + if (userProfile.avatarUrlPath.length < 1) { + OWSFail(@"%@ Malformed avatar URL: %@", self.tag, userProfile.avatarUrlPath); return; } - if (userProfile.profileKey.keyData.length < 1 || userProfile.avatarUrl.length < 1) { + if (userProfile.profileKey.keyData.length < 1 || userProfile.avatarUrlPath.length < 1) { return; } @@ -773,8 +771,8 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; NSString *tempDirectory = NSTemporaryDirectory(); NSString *tempFilePath = [tempDirectory stringByAppendingPathComponent:fileName]; - NSURL *avatarUrl = [NSURL URLWithString:userProfile.avatarUrl relativeToURL:self.avatarHTTPManager.baseURL]; - NSURLRequest *request = [NSURLRequest requestWithURL:avatarUrl]; + NSURL *avatarUrlPath = [NSURL URLWithString:userProfile.avatarUrlPath relativeToURL:self.avatarHTTPManager.baseURL]; + NSURLRequest *request = [NSURLRequest requestWithURL:avatarUrlPath]; NSURLSessionDownloadTask *downloadTask = [self.avatarHTTPManager downloadTaskWithRequest:request progress:^(NSProgress *_Nonnull downloadProgress) { DDLogVerbose(@"%@ Downloading avatar for %@", self.tag, userProfile.recipientId); @@ -862,7 +860,7 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; - (void)updateProfileForRecipientId:(NSString *)recipientId profileNameEncrypted:(nullable NSData *)profileNameEncrypted - avatarUrl:(nullable NSString *)avatarUrl; + avatarUrlPath:(nullable NSString *)avatarUrlPath; { OWSAssert(recipientId.length > 0); @@ -877,17 +875,17 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; NSString *_Nullable profileName = [self decryptProfileNameData:profileNameEncrypted profileKey:userProfile.profileKey]; - BOOL isAvatarSame = [self isNullableStringEqual:userProfile.avatarUrl toString:avatarUrl]; + BOOL isAvatarSame = [self isNullableStringEqual:userProfile.avatarUrlPath toString:avatarUrlPath]; dispatch_async(dispatch_get_main_queue(), ^{ userProfile.profileName = profileName; - userProfile.avatarUrl = avatarUrl; + userProfile.avatarUrlPath = avatarUrlPath; if (!isAvatarSame) { // Evacuate avatar image cache. [self.otherUsersProfileAvatarImageCache removeObjectForKey:recipientId]; - if (avatarUrl) { + if (avatarUrlPath) { [self downloadAvatarForUserProfile:userProfile]; } } @@ -993,6 +991,8 @@ static const NSUInteger kOWSProfileManager_NameDataLength = 26; NSUInteger paddingByteCount = kOWSProfileManager_NameDataLength - nameData.length; NSMutableData *paddedNameData = [nameData mutableCopy]; + // Since we want all encrypted profile names to be the same length on the server, we use `increaseLengthBy` + // to pad out any remaining length with 0 bytes. [paddedNameData increaseLengthBy:paddingByteCount]; OWSAssert(paddedNameData.length == kOWSProfileManager_NameDataLength); diff --git a/Signal/src/Profiles/ProfileFetcherJob.swift b/Signal/src/Profiles/ProfileFetcherJob.swift index b105fc4d8..9e32c359d 100644 --- a/Signal/src/Profiles/ProfileFetcherJob.swift +++ b/Signal/src/Profiles/ProfileFetcherJob.swift @@ -116,7 +116,7 @@ class ProfileFetcherJob: NSObject { OWSProfileManager.shared().updateProfile(forRecipientId: signalServiceProfile.recipientId, profileNameEncrypted: signalServiceProfile.profileNameEncrypted, - avatarUrl: signalServiceProfile.avatarUrl) + avatarUrlPath: signalServiceProfile.avatarUrlPath) } private func verifyIdentityUpToDateAsync(recipientId: String, latestIdentityKey: Data) { @@ -138,13 +138,12 @@ struct SignalServiceProfile { case invalid(description: String) case invalidIdentityKey(description: String) case invalidProfileName(description: String) - case invalidAvatarUrl(description: String) } public let recipientId: String public let identityKey: Data public let profileNameEncrypted: Data? - public let avatarUrl: String? + public let avatarUrlPath: String? init(recipientId: String, rawResponse: Any?) throws { self.recipientId = recipientId @@ -173,7 +172,7 @@ struct SignalServiceProfile { self.profileNameEncrypted = nil } - self.avatarUrl = responseDict["avatar"] as? String + self.avatarUrlPath = responseDict["avatar"] as? String // `removeKeyType` is an objc category method only on NSData, so temporarily cast. self.identityKey = (identityKeyWithType as NSData).removeKeyType() as Data diff --git a/SignalServiceKit/src/Network/API/Requests/TSRequest.h b/SignalServiceKit/src/Network/API/Requests/TSRequest.h index 3c7fd1383..3b032e9c4 100644 --- a/SignalServiceKit/src/Network/API/Requests/TSRequest.h +++ b/SignalServiceKit/src/Network/API/Requests/TSRequest.h @@ -10,9 +10,4 @@ - (void)makeAuthenticatedRequest; -#pragma mark - Factory methods - -// move to builder class/header -+ (instancetype)setProfileNameRequestWithProfileName:(NSString *)encryptedName; - @end diff --git a/SignalServiceKit/tests/TestSupport/Fakes/OWSFakeProfileManager.m b/SignalServiceKit/tests/TestSupport/Fakes/OWSFakeProfileManager.m index f9f12b410..53bb78b4b 100644 --- a/SignalServiceKit/tests/TestSupport/Fakes/OWSFakeProfileManager.m +++ b/SignalServiceKit/tests/TestSupport/Fakes/OWSFakeProfileManager.m @@ -3,20 +3,24 @@ // #import "OWSFakeProfileManager.h" +#import "Cryptography.h" #import "TSThread.h" NS_ASSUME_NONNULL_BEGIN @interface OWSFakeProfileManager () -@property (nonatomic, readonly) NSMutableDictionary *profileKeys; +@property (nonatomic, readonly) NSMutableDictionary *profileKeys; @property (nonatomic, readonly) NSMutableSet *recipientWhitelist; @property (nonatomic, readonly) NSMutableSet *threadWhitelist; +@property (nonatomic, readonly) OWSAES128Key *localProfileKey; @end @implementation OWSFakeProfileManager +@synthesize localProfileKey = _localProfileKey; + - (instancetype)init { self = [super init]; @@ -32,14 +36,19 @@ NS_ASSUME_NONNULL_BEGIN } -- (NSData *)localProfileKey +- (OWSAES128Key *)localProfileKey { - return [@"fake-local-profile-key-for-testing" dataUsingEncoding:NSUTF8StringEncoding]; + if (_localProfileKey == nil) { + _localProfileKey = [OWSAES128Key generateRandomKey]; + } + return _localProfileKey; } -- (void)setProfileKey:(NSData *)profileKey forRecipientId:(NSString *)recipientId +- (void)setProfileKeyData:(NSData *)profileKey forRecipientId:(NSString *)recipientId { - self.profileKeys[recipientId] = profileKey; + OWSAES128Key *key = [OWSAES128Key keyWithData:profileKey]; + NSAssert(key, @"Unable to build key. Invalid key data?"); + self.profileKeys[recipientId] = key; } - (BOOL)isUserInProfileWhitelist:(NSString *)recipientId