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

RSA4c #519

Merged
merged 14 commits into from
Dec 12, 2016
Merged

RSA4c #519

Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Source/ARTAuth+Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ ART_ASSUME_NONNULL_BEGIN
@property (nonatomic, readonly, assign) NSTimeInterval timeOffset;

@property (art_nullable, weak) id<ARTAuthDelegate> delegate;
@property (readonly, assign) BOOL authorizing;

@end

Expand Down
3 changes: 3 additions & 0 deletions Source/ARTAuth.m
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ - (void)authorize:(ARTTokenParams *)tokenParams options:(ARTAuthOptions *)authOp
if (callback) {
callback(self.tokenDetails, nil);
}
_authorizing = false;
};

// Failure
Expand All @@ -360,6 +361,7 @@ - (void)authorize:(ARTTokenParams *)tokenParams options:(ARTAuthOptions *)authOp
if (callback) {
callback(nil, error);
}
_authorizing = false;
};

__weak id<ARTAuthDelegate> lastDelegate = self.delegate;
Expand All @@ -378,6 +380,7 @@ - (void)authorize:(ARTTokenParams *)tokenParams options:(ARTAuthOptions *)authOp

// Request always a new token
[self.logger verbose:@"RS:%p ARTAuth: requesting new token.", _rest];
_authorizing = true;
[self requestToken:currentTokenParams withOptions:replacedOptions callback:^(ARTTokenDetails *tokenDetails, NSError *error) {
if (error) {
failureBlock(error);
Expand Down
3 changes: 1 addition & 2 deletions Source/ARTAuthOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ - (id)copyWithZone:(NSZone *)zone {
}

- (NSString *)description {
return [NSString stringWithFormat: @"%@: key=%@ token=%@ authUrl=%@ authMethod=%@ hasAuthCallback=%d",
NSStringFromClass([self class]), self.key, self.token, self.authUrl, self.authMethod, self.authCallback != nil];
return [NSString stringWithFormat:@"%@ - \n\t key: %@; \n\t token: %@; \n\t authUrl: %@; \n\t authMethod: %@; \n\t hasAuthCallback: %d;", [super description], self.key, self.token, self.authUrl, self.authMethod, self.authCallback != nil];
}

- (NSString *)token {
Expand Down
4 changes: 4 additions & 0 deletions Source/ARTClientOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ - (instancetype)initDefaults {
return self;
}

- (NSString *)description {
return [NSString stringWithFormat:@"%@\n\t clientId: %@;", [super description], self.clientId];
}

- (NSString*)getRestHost {
if (_restHost) {
return _restHost;
Expand Down
3 changes: 3 additions & 0 deletions Source/ARTGCD.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@

void artDispatchSpecifyMainQueue();
void artDispatchMainQueue(dispatch_block_t block);
void artDispatchGlobalQueue(dispatch_block_t block);
dispatch_block_t artDispatchScheduled(NSTimeInterval seconds, dispatch_block_t block);
void artDispatchCancel(dispatch_block_t block);

#endif /* ARTGCD_h */
18 changes: 15 additions & 3 deletions Source/ARTGCD.m
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,20 @@ void artDispatchMainQueue(dispatch_block_t block) {
block();
}
else {
dispatch_async(dispatch_get_main_queue(), ^{
block();
});
dispatch_async(dispatch_get_main_queue(), block);
}
}

void artDispatchGlobalQueue(dispatch_block_t block) {
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), block);
}

dispatch_block_t artDispatchScheduled(NSTimeInterval seconds, dispatch_block_t block) {
dispatch_block_t work = dispatch_block_create(0, block);
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(NSEC_PER_SEC * seconds)), dispatch_get_main_queue(), work);
return work;
}

void artDispatchCancel(dispatch_block_t block) {
dispatch_block_cancel(block);
}
110 changes: 96 additions & 14 deletions Source/ARTRealtime.m
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#import "ARTRealtimeTransport.h"
#import "ARTFallback.h"
#import "ARTAuthDetails.h"
#import "ARTGCD.h"

@interface ARTConnectionStateChange ()

Expand Down Expand Up @@ -153,7 +154,20 @@ - (NSString *)getClientId {
}

- (NSString *)description {
return [NSString stringWithFormat:@"Realtime: %@", self.clientId];
NSString *info;
if (self.options.token) {
info = [NSString stringWithFormat:@"token: %@", self.options.token];
}
else if (self.options.authUrl) {
info = [NSString stringWithFormat:@"authUrl: %@", self.options.authUrl];
}
else if (self.options.authCallback) {
info = [NSString stringWithFormat:@"authCallback: %@", self.options.authCallback];
}
else {
info = [NSString stringWithFormat:@"key: %@", self.options.key];
}
return [NSString stringWithFormat:@"%@ - \n\t %@;", [super description], info];
}

- (ARTAuth *)getAuth {
Expand Down Expand Up @@ -268,11 +282,12 @@ - (void)transition:(ARTRealtimeConnectionState)state withErrorInfo:(ARTErrorInfo

- (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
ARTStatus *status = nil;
__weak __typeof(self) weakSelf = self;
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of things could really use an explanatory comment. I have no idea why this change. I mean, I assume you don't want to increase the reference count in all those callback blocks but I'm not sure if there's some subtle reason for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, I don't want to increase the ref count on those blocks because, for example, the unlessStateChangesBefore is setting a timer and if the ARTRealtime instance is released before that timer, then it could create a retain cycle and then we have a leak. That could also bring some instability in the test suite because we are creating a lot of ARTRealtime instances. I will add a comment about it. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 6939c90.


switch (stateChange.current) {
case ARTRealtimeConnecting: {
[self unlessStateChangesBefore:[ARTDefault realtimeRequestTimeout] do:^{
[self transition:ARTRealtimeDisconnected withErrorInfo:[ARTErrorInfo createWithCode:0 status:ARTStateConnectionFailed message:@"timed out"]];
[weakSelf onConnectionTimeOut];
}];

if (!_reachability) {
Expand All @@ -295,19 +310,19 @@ - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
if (self.connection.state != ARTRealtimeFailed && self.connection.state != ARTRealtimeClosed && self.connection.state != ARTRealtimeDisconnected) {
[_reachability listenForHost:[_transport host] callback:^(BOOL reachable) {
if (reachable) {
switch (_connection.state) {
switch ([[weakSelf connection] state]) {
case ARTRealtimeDisconnected:
case ARTRealtimeSuspended:
[self transition:ARTRealtimeConnecting];
[weakSelf transition:ARTRealtimeConnecting];
default:
break;
}
} else {
switch (_connection.state) {
switch ([[weakSelf connection] state]) {
case ARTRealtimeConnecting:
case ARTRealtimeConnected: {
ARTErrorInfo *unreachable = [ARTErrorInfo createWithCode:-1003 message:@"unreachable host"];
[self transition:ARTRealtimeDisconnected withErrorInfo:unreachable];
[weakSelf transition:ARTRealtimeDisconnected withErrorInfo:unreachable];
break;
}
default:
Expand All @@ -322,7 +337,7 @@ - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
case ARTRealtimeClosing: {
[_reachability off];
[self unlessStateChangesBefore:[ARTDefault realtimeRequestTimeout] do:^{
[self transition:ARTRealtimeClosed];
[weakSelf transition:ARTRealtimeClosed];
}];
[self.transport sendClose];
break;
Expand Down Expand Up @@ -365,7 +380,7 @@ - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
[stateChange setRetryIn:self.options.disconnectedRetryTimeout];

[self unlessStateChangesBefore:stateChange.retryIn do:^{
[self transition:ARTRealtimeConnecting];
[weakSelf transition:ARTRealtimeConnecting];
}];

break;
Expand All @@ -376,7 +391,7 @@ - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
_transport = nil;
[stateChange setRetryIn:self.options.suspendedRetryTimeout];
[self unlessStateChangesBefore:stateChange.retryIn do:^{
[self transition:ARTRealtimeConnecting];
[weakSelf transition:ARTRealtimeConnecting];
}];
[_authorizationEmitter emit:[NSNumber numberWithInt:ARTAuthorizationFailed] with:[ARTErrorInfo createWithCode:ARTStateAuthorizationFailed message:@"Connection has been suspended"]];
break;
Expand Down Expand Up @@ -433,6 +448,24 @@ - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
[self.connection emit:stateChange.current with:stateChange];
}

- (void)onConnectionTimeOut {
NSInteger errorCode;
if (self.auth.authorizing && (self.options.authUrl || self.options.authCallback)) {
errorCode = ARTCodeErrorAuthConfiguredProviderFailure;
}
else {
errorCode = ARTCodeErrorConnectionTimedOut;
}
switch (self.connection.state) {
case ARTRealtimeConnected:
[self transition:ARTRealtimeConnected withErrorInfo:[ARTErrorInfo createWithCode:errorCode status:ARTStateConnectionFailed message:@"timed out"]];
break;
default:
[self transition:ARTRealtimeDisconnected withErrorInfo:[ARTErrorInfo createWithCode:errorCode status:ARTStateConnectionFailed message:@"timed out"]];
break;
}
}

- (void)unlessStateChangesBefore:(NSTimeInterval)deadline do:(void(^)())callback {
// Defer until next event loop execution so that any event emitted in the current
// one doesn't cancel the timeout.
Expand Down Expand Up @@ -536,13 +569,26 @@ - (void)onClosed {
}
}

- (void)onAuth {
[self.logger info:@"R:%p server has requested an authorise", self];
switch (self.connection.state) {
case ARTRealtimeConnecting:
case ARTRealtimeConnected:
[self transportReconnectWithRenewedToken];
break;
default:
NSAssert(false, @"Invalid Realtime state: expected Connecting or Connected");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would reserve those assertions for bugs on the library. If e. g. the server sends an AUTH while the connection is closing, that would be a bug in the server, and we probably should treat those more gracefully than crashing, e. g. log and ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done b6d6f92.

break;
}
}

- (void)onError:(ARTProtocolMessage *)message {
// TODO work out which states this can be received in
if (message.channel) {
[self onChannelMessage:message];
} else {
ARTErrorInfo *error = message.error;
if ([self shouldRenewToken:&error]) {
[self.transport close];
Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's closed again at 621?

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 can't precise the problem that was occurring if the close wasn't called explicitly but the main reason I added the close call was because the authorize from transportReconnectWithRenewedToken takes time and the client shouldn't use the current token.

[self transportReconnectWithRenewedToken];
return;
}
Expand All @@ -567,12 +613,43 @@ - (void)transportReconnectWithHost:(NSString *)host {
[self.transport connect];
}

- (void)transportReconnectWithRenewedToken {
_renewingToken = true;
- (void)transportReconnectWithTokenDetails:(ARTTokenDetails *)tokenDetails error:(NSError *)error {
if (error && (self.options.authUrl || self.options.authCallback)) {
[self.connection setErrorReason:[ARTErrorInfo createWithCode:ARTCodeErrorAuthConfiguredProviderFailure message:error.localizedDescription]];
return;
}
[_transport close];
_transport = [[_transportClass alloc] initWithRest:self.rest options:self.options resumeKey:_transport.resumeKey connectionSerial:_transport.connectionSerial];
_transport.delegate = self;
[_transport connectForcingNewToken:true];
[_transport connectWithToken:tokenDetails.token error:error];
}

- (void)transportReconnectWithRenewedToken {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing something, all this logic will only apply when transportReconnectWithRenewedToken is called, ie. when an AUTH or ERROR is received, but not when e. g. first connection or manually calling authorize. The spec says "If an attempt by the realtime client library to authenticate is made using...", so every auth attempt should follow this logic. I'd say this logic should be inside Auth.authorize itself, don't you think?

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 point 👍

Copy link
Member

Choose a reason for hiding this comment

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

@ricardopereira have you made the change on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattheworiordan No, working on it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done efa57ab.

Copy link
Contributor

Choose a reason for hiding this comment

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

That one broke some tests, unfortunately...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😔 Yes, I'm on it. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 3462531.

self.auth.delegate = nil;
@try {
_renewingToken = true;
__weak __typeof(self) weakSelf = self;
dispatch_block_t work = artDispatchScheduled([ARTDefault realtimeRequestTimeout], ^{
[weakSelf onConnectionTimeOut];
});
[self.auth authorize:nil options:self.options callback:^(ARTTokenDetails *tokenDetails, NSError *error) {
// Cancel scheduled work
artDispatchCancel(work);
// It's still valid?
switch ([[weakSelf connection] state]) {
case ARTRealtimeClosing:
case ARTRealtimeClosed:
return;
default:
break;
}
[[weakSelf getLogger] debug:__FILE__ line:__LINE__ message:@"R:%p WS:%p authorised: %@ error: %@", weakSelf, [weakSelf getTransport], tokenDetails, error];
[weakSelf transportReconnectWithTokenDetails:tokenDetails error:error];
}];
}
@finally {
self.auth.delegate = self;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also no idea why this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ARTAuth doesn't know about ARTRealtime so it has a delegate (ARTAuthDelegate). ARTRealtime is using that delegate but in that case it should not use the delegate functionalities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, your comment above is relevant about this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done efa57ab.

}
}

- (void)onAck:(ARTProtocolMessage *)message {
Expand All @@ -584,7 +661,9 @@ - (void)onNack:(ARTProtocolMessage *)message {
}

- (void)onChannelMessage:(ARTProtocolMessage *)message {
// TODO work out which states this can be received in / error info?
if (message.channel == nil) {
return;
}
ARTRealtimeChannel *channel = [self.channels get:message.channel];
[channel onChannelMessage:message];
}
Expand Down Expand Up @@ -886,6 +965,9 @@ - (void)realtimeTransport:(id)transport didReceiveMessage:(ARTProtocolMessage *)
case ARTProtocolMessageClosed:
[self onClosed];
break;
case ARTProtocolMessageAuth:
[self onAuth];
break;
default:
[self onChannelMessage:message];
break;
Expand Down
4 changes: 2 additions & 2 deletions Source/ARTRealtimeTransport.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ typedef NS_ENUM(NSUInteger, ARTRealtimeTransportState) {
- (void)send:(ARTProtocolMessage *)msg;
- (void)receive:(ARTProtocolMessage *)msg;
- (void)connect;
- (void)connectWithToken:(NSString *)token; //?!
- (void)connectForcingNewToken:(BOOL)forceNewToken;
- (void)connectWithToken:(NSString *)token;
- (void)connectWithToken:(NSString *)token error:(art_nullable NSError *)error;
- (void)sendClose;
- (void)sendPing;
- (void)close;
Expand Down
17 changes: 17 additions & 0 deletions Source/ARTRest.m
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,23 @@ - (void)dealloc {
[self.logger debug:__FILE__ line:__LINE__ message:@"RS:%p dealloc", self];
}

- (NSString *)description {
NSString *info;
if (self.options.token) {
info = [NSString stringWithFormat:@"token: %@", self.options.token];
}
else if (self.options.authUrl) {
info = [NSString stringWithFormat:@"authUrl: %@", self.options.authUrl];
}
else if (self.options.authCallback) {
info = [NSString stringWithFormat:@"authCallback: %@", self.options.authCallback];
}
else {
info = [NSString stringWithFormat:@"key: %@", self.options.key];
}
return [NSString stringWithFormat:@"%@ - \n\t %@;", [super description], info];
}

- (void)executeRequest:(NSMutableURLRequest *)request withAuthOption:(ARTAuthentication)authOption completion:(void (^)(NSHTTPURLResponse *__art_nullable, NSData *__art_nullable, NSError *__art_nullable))callback {
request.URL = [NSURL URLWithString:request.URL.relativeString relativeToURL:self.baseUrl];

Expand Down
4 changes: 3 additions & 1 deletion Source/ARTStatus.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ typedef NS_ENUM(NSUInteger, ARTState) {
*/
typedef CF_ENUM(NSUInteger, ARTCodeError) {
// FIXME: check hard coded errors
ARTCodeErrorAPIKeyMissing = 80001
ARTCodeErrorAPIKeyMissing = 80001,
ARTCodeErrorConnectionTimedOut = 80014,
ARTCodeErrorAuthConfiguredProviderFailure = 80019
};

ART_ASSUME_NONNULL_BEGIN
Expand Down
3 changes: 3 additions & 0 deletions Source/ARTTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,7 @@ NSString *generateNonce();
@interface NSDictionary (ARTJsonCompatible) <ARTJsonCompatible>
@end

@interface NSURL (ARTLog)
@end

ART_ASSUME_NONNULL_END
8 changes: 8 additions & 0 deletions Source/ARTTypes.m
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,11 @@ - (NSDictionary *)toJSON:(NSError *__art_nullable *__art_nullable)error {
}

@end

@implementation NSURL (ARTLog)

- (NSString *)description {
return [NSString stringWithFormat:@"%@", self.absoluteString];
}

@end
Loading