RSA10 #94

ricardopereira · 2015-11-27T12:50:50Z

No description provided.

ricardopereira · 2015-11-27T16:02:11Z

(RSA10c) Will not create a new token unless no previous token exists or current token has expired.

Not implemented:

Please note that a buffer of 15s for token expiry is recommended to avoid race conditions where the token is valid at the time of the request, but invalid when it reaches the server. Therefore, we recommend that a token is considered expired 15s before the time field expires

ricardopereira · 2015-11-27T17:29:49Z

(RSA10e) Adheres to the implementation of requestToken when issuing new tokens

I need to think about it. Right now I do not have a clue how to achieve this.

ricardopereira · 2015-11-27T17:45:38Z

Not implemented:

(RSA10g) Stores the AuthOptions and TokenParams arguments as defaults for subsequent authorisations

mattheworiordan · 2015-12-02T21:35:35Z

(RSA10c) Will not create a new token unless no previous token exists or current token has expired.

Not implemented:
Please note that a buffer of 15s for token expiry is recommended to avoid race conditions where the token is valid at the time of the request, but invalid when it reaches the server. Therefore, we recommend that a token is considered expired 15s before the time field expires

@ricardopereira that is fine, but why?

(RSA10e) Adheres to the implementation of requestToken when issuing new tokens

I need to think about it. Right now I do not have a clue how to achieve this.

Surely you just need to have a test that proves that requesting a token through an authorise call leads to a requestToken call with the arguments provided? If you do that, then it adheres to the spec.

Not implemented:
(RSA10g) Stores the AuthOptions and TokenParams arguments as defaults for subsequent authorisations

Why not? This is quite an important requirement.

mattheworiordan · 2015-12-02T21:36:55Z

ably-ios/ARTAuth.m

    BOOL requestNewToken = NO;
+    BOOL useTheForce = options ? options.force : false;


useTheForce? Sounds like something out of Star Wars. Why not simply forceTokenRenewal?

mattheworiordan · 2015-12-02T21:49:00Z

A few comments, but largely good progress, 👍

ricardopereira · 2016-01-04T21:27:54Z

@mattheworiordan Done

ricardopereira · 2016-01-05T16:03:17Z

Tests are failing because of bug #114. (test Auth.swift#L801)
Should I fix?

mattheworiordan · 2016-01-07T09:31:40Z

Tests are failing because of bug #114. (test Auth.swift#L801)
Should I fix?

Well up to you, it would be nice to not introduce PRs that change the build from green to red. I think you should look at fixing the issue as part of this PR so that this PR passes, but do no more than necessary, and anything else should go in a separate PR to address the issue. Alternatively, and probably better, perhaps fix #114 in a new PR, and then rebase this one on top of that afterwards.

mattheworiordan · 2016-01-07T09:32:42Z

ably-ios/ARTAuth.h

@@ -30,7 +30,7 @@ ART_ASSUME_NONNULL_BEGIN
 @property (nonatomic, weak) ARTLog *logger;

 @property (art_nullable, readonly, getter=getClientId) NSString *clientId;
-@property (art_nullable, nonatomic, readonly, strong) ARTAuthTokenDetails *tokenDetails;
+@property (art_nullable, nonatomic, readwrite, strong) ARTAuthTokenDetails *tokenDetails;


Why is this readwrite? I assume this means a consumer of this API could explicitly update the tokenDetails without using the correct API. Is this intentional? If so, why?

It is a mistake. I removed the setTokenDetails and I change it to readwrite just to compile the code successfully and continue the remaining work of this spec.

mattheworiordan · 2016-01-07T11:30:11Z

Few comments, but mostly looks good

ricardopereira · 2016-01-08T23:45:29Z

Needs #123.

mattheworiordan · 2016-01-14T19:42:29Z

ably-ios/ARTAuth.m

    if (mergedOptions.authUrl) {
        NSMutableURLRequest *request = [self buildRequest:mergedOptions withParams:currentTokenParams];

        [self.logger debug:__FILE__ line:__LINE__ message:@"using authUrl (%@ %@)", request.HTTPMethod, request.URL];

-        [_rest executeRequest:request withAuthOption:ARTAuthenticationUseBasic completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) {
+        [_rest executeRequest:request withAuthOption:ARTAuthenticationOff completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) {


Why ARTAuthenticationOff, what does that mean?

ARTAuthenticationOff means the host does not require authentication (no Authorization http header)

mattheworiordan · 2016-01-14T19:50:44Z

Few small comments, please address and will merge. Also, this won't merge automatically now unfottunately

- It is possible to request a token either with an API key, authCallback or authUrl

- The `authCallback` was calling the wrong callback.

- Generate a token request with one RestClient and one other to test the `authCallback`

The token retrieved is assumed by the library to be a token string if the response has Content-Type "text/plain", or taken to be a TokenRequest or TokenDetails object if the response has Content-Type "application/json"

- added `ARTJsonEncoder.encodeTokenDetails` to use with echo.ably.io service - added `getTestTokenDetails()`

…en return an error with HTTP StatusCode

- Added custom nimble matcher `beCloseTo` for NSDates

ricardopereira · 2016-01-15T11:58:10Z

Rebased with one change c670e8f.

mattheworiordan · 2016-01-15T14:14:10Z

LGTM

tcard · 2016-01-15T14:34:44Z

LGTM

RSA10

mattheworiordan reviewed Dec 2, 2015
View reviewed changes

ricardopereira force-pushed the RSA10 branch from c74f05c to 4ff4366 Compare January 5, 2016 15:22

mattheworiordan reviewed Jan 7, 2016
View reviewed changes

ricardopereira force-pushed the RSA10 branch from 9d9b367 to 4c71dc3 Compare January 9, 2016 07:57

mattheworiordan reviewed Jan 14, 2016
View reviewed changes

ricardopereira added 4 commits January 15, 2016 10:05

RSA10a

a09b342

RSA10b

03b224d

RSA10c

9de81fd

RSA10d. Removed force argument from authorise

d4ddbac

ricardopereira added 22 commits January 15, 2016 10:16

ARTAuth.requestToken: removed wrong validation

62b300c

- It is possible to request a token either with an API key, authCallback or authUrl

Fixed use of authCallback on Auth.requestToken

d6164d3

- The `authCallback` was calling the wrong callback.

Fixed RSA10i implementation

a93c68f

- Generate a token request with one RestClient and one other to test the `authCallback`

RSA10a: initialises with the token and verifies if Token auth is used

09aa9e9

RSA10g

a252536

RSA10e

20c4d50

Implement: handleAuthUrlResponse

cbf0f85

The token retrieved is assumed by the library to be a token string if the response has Content-Type "text/plain", or taken to be a TokenRequest or TokenDetails object if the response has Content-Type "application/json"

RSA10i: authUrl

1940983

- added `ARTJsonEncoder.encodeTokenDetails` to use with echo.ably.io service - added `getTestTokenDetails()`

Fix ARTAuth.buildURL: was always clearing the url query items

a63d414

Fix ARTRest.executeRequest: if ARTErrorStatusCode does not exist th…

629319e

…en return an error with HTTP StatusCode

RSA10i: TokenParams

ab7346b

Enhance: for now, encodeTokenDetails can return nil

2243995

Fix RSA9: "createRequestToken"

af4a52e

Fix ARTAuth.tokenDetails: back to readonly

b509db6

Fix RSC9: auth.tokenDetails is readonly

316b29e

Fix RSC10: auth.tokenDetails is readonly

a6c239e

Enhance RSA10c: deterministic test

61aa5ee

Enhance RSA10d: more expressive

81754f8

Enhance RSA10g: additional TokenParams attributes

6c7f42b

- Added custom nimble matcher `beCloseTo` for NSDates

Enhance RSA10i: additional TokenParams attributes

9d461dd

Enhance RSA10g: more test cases

26d8635

RSA10j

7909ee7

ricardopereira force-pushed the RSA10 branch from e00fad4 to 7909ee7 Compare January 15, 2016 10:16

Enhance RSA10: use of guard-let

c670e8f

tcard added a commit that referenced this pull request Jan 15, 2016

Merge pull request #94 from ably/RSA10

a434a5f

RSA10

tcard merged commit a434a5f into master Jan 15, 2016

ricardopereira deleted the RSA10 branch January 19, 2016 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSA10 #94

RSA10 #94

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

mattheworiordan commented Dec 2, 2015

mattheworiordan Dec 2, 2015

mattheworiordan commented Dec 2, 2015

ricardopereira commented Jan 4, 2016

ricardopereira commented Jan 5, 2016

mattheworiordan commented Jan 7, 2016

mattheworiordan Jan 7, 2016

ricardopereira Jan 7, 2016

mattheworiordan commented Jan 7, 2016

ricardopereira commented Jan 8, 2016

mattheworiordan Jan 14, 2016

ricardopereira Jan 14, 2016

mattheworiordan commented Jan 14, 2016

ricardopereira commented Jan 15, 2016

mattheworiordan commented Jan 15, 2016

tcard commented Jan 15, 2016

		BOOL requestNewToken = NO;
		BOOL useTheForce = options ? options.force : false;

RSA10 #94

RSA10 #94

Conversation

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

ricardopereira commented Nov 27, 2015

mattheworiordan commented Dec 2, 2015

mattheworiordan Dec 2, 2015

Choose a reason for hiding this comment

mattheworiordan commented Dec 2, 2015

ricardopereira commented Jan 4, 2016

ricardopereira commented Jan 5, 2016

mattheworiordan commented Jan 7, 2016

mattheworiordan Jan 7, 2016

Choose a reason for hiding this comment

ricardopereira Jan 7, 2016

Choose a reason for hiding this comment

mattheworiordan commented Jan 7, 2016

ricardopereira commented Jan 8, 2016

mattheworiordan Jan 14, 2016

Choose a reason for hiding this comment

ricardopereira Jan 14, 2016

Choose a reason for hiding this comment

mattheworiordan commented Jan 14, 2016

ricardopereira commented Jan 15, 2016

mattheworiordan commented Jan 15, 2016

tcard commented Jan 15, 2016