RSA10j #492

EvgenyKarkan · 2016-09-04T16:10:01Z

c5e461b

mattheworiordan · 2016-09-07T10:54:53Z

Spec/Auth.swift

@@ -1903,6 +1903,32 @@ class Auth : QuickSpec {
                        done()
                    }
                }
+


I appreciate the code above was not yours, but I do not follow the logic. The queryTime option is set to true for AuthOptions, I get that. But the time from the time method is stored and then the authorise request is then executed and the issued attribute is checked to be within 1 second. Firstly, if we have even a minor delay here this test will fail, but I am not sure what it's even testing. If we want to check the time then we should have a test for http://docs.ably.io/client-lib-development-guide/features/#RSA10k, but this is mixing concerns and is brittle. Please can you confirm why you think this is even here, and whether RSA10k has suitable coverage proving the request for time is made elsewhere i.e. we can then remove this.

I guess that previous developer just wanted to test that issued time is in 1 second range with server time, but I also cannot understand the pros of doing time function call to store server time.

Ok, well let's remove then and if this is a concern perhaps add as a separate test such as localClockInSync which simply fails with "Local clock is out of sync with Ably so tests may have unpredictable results"

tcard · 2016-09-07T21:14:36Z

Please try to keep independent features separate. This work and RSA9h (#491) are independent and should both be built upon RSA8e (#487). Now if you change RSA9h you also have to change this, and we can't merge this without merging RSA9h.

EvgenyKarkan · 2016-09-08T14:41:39Z

Please try to keep independent features separate. ...
Now if you change RSA9h you also have to change this, and we can't merge this without merging RSA9h.

I'll keep it in mind, thanks. I'll merge RSA9h into current branch.

mattheworiordan · 2016-09-09T12:50:22Z

Source/ARTAuth.m

@@ -263,9 +263,9 @@ - (void)executeTokenRequest:(ARTTokenRequest *)tokenRequest callback:(void (^)(A

 - (void)authorise:(ARTTokenParams *)tokenParams options:(ARTAuthOptions *)authOptions callback:(void (^)(ARTTokenDetails *, NSError *))callback {
    BOOL requestNewToken = NO;
-    ARTAuthOptions *mergedOptions = [self mergeOptions:authOptions];
+    ARTAuthOptions *mergedOptions = authOptions ? authOptions : self.options;


Name is a bit misleading, perhap just currentAuthOptions inline with currentTokenParams below?

Oh, sorry, of course :)

EvgenyKarkan · 2016-09-13T15:50:01Z

Please review and merge if everything looks good

tcard · 2016-09-13T23:59:14Z

Spec/Auth.swift

@@ -1717,14 +1718,12 @@ class Auth : QuickSpec {
                    waitUntil(timeout: testTimeout) { done in
                        auth.authorise(nil, options: authOptions) { tokenDetails, error in
                            expect(authCallbackHasBeenInvoked).to(beTrue())
-
-                            authCallbackHasBeenInvoked = false


This test shouldn't be changed. It should work, or else we're breaking RSA10g.

This test shouldn't be changed

Sorry but why?
If I leave the test as it was before - it will fail, because of authorise implementation changes (replace instead of merge). authorise function stores options and params - so I guess I just need to modify test to make it green, no?

Because @EvgenyKarkan I believe what @tcard is saying is that you should test that the defaults are reused first (i.e. Stores the AuthOptions and TokenParams arguments as defaults for subsequent authorisations with the exception of the attributes TokenParams#timestamp and AuthOptions#queryTime). The test for http://docs.ably.io/client-lib-development-guide/features/#RSA10g is now missing i.e. we need to ensure that the defaults are carried through to the following request for RSA10g, and for RSA10j (this PR), you need to ensure any provided details override existing defaults. Added to this, https://github.com/ably/ably-ios/pull/492/files#diff-b5d84fe9bce26902a7481c2ffec84fffR1723 is really not relevant given that param has been removed completely given RSA10d is now removed, see https://github.com/ably/docs/blob/source/content/client-lib-development-guide/versions/features-0-8__0-9.diff#L116.

So in summary:

Test that defaults are carried through when none are provided (need initial + subsequent request without any arguments)

Test that defaults are changed when set (need initial + subsequent request without any arguments)

Remove force completely as this just confuses these tests given that option is no longer relevant and should be removed.

Test that defaults are changed when set (need initial + subsequent request without any arguments)

I guess you mean + subsequent requestwithany arguments here?

Remove force completely

Remove it only from tests target, or also from the lib target?

I guess you mean + subsequent requestwithany arguments here?

No, we need to test that the defaults are used for subsequent authorisations so if we passed in arguments then we'd be changing those defaults?

Remove force completely

Remove it only from tests target, or also from the lib target?

Not sure what you mean by target, but it should be removed as this option is no longer available as part of the tests you are implementing

No, we need to test that the defaults are used for subsequent authorisations so if we passed in arguments then we'd be changing those defaults?

I might be missing something, but as per RSA10j spec that states:

For example, if a client is initialised with TokenParams#ttl configured with a custom value, and a TokenParams object is passed in as an argument to authorise with a null value for ttl, then the ttl used for every subsequent authorisation will be null.

Does not this mean that the default parameters need to be replaced?

Yes but if no arguments are passed in, then a TokenParams object has not been passed in.

What it actually states is:

TokenParams and AuthOptions are optional. When the arguments are present...

The point is that if you don't provide those arguments or they are null (but not empty TokenParam or AuthObtion objects), then the defaults are not overriden.

Not sure what you mean by target

Well, Xcode project can contain several build targets - e.g.. main iOS or OSX app, unit tests, apple watch app, app extension, framework, etc.

In our case we have Ably ObjC library target, AblySpecs Swift tests target, and AblyTests ObjC tests target.

So my question was, in another words, - should I remove use of force from tests only, or should I remove force property from AuthOptions class in Ably library implementation too?

I guess I just need to remove it from test target only, otherwise it won't be possible to implement RSA10d and RTC8, because both of them needs force.

Am I right here, could you confirm please @mattheworiordan ?

tcard · 2016-09-14T00:01:00Z

@EvgenyKarkan I've rebased this PR on top of master instead of RSA9h, so that they are independent. It was getting pretty hard to review. Sorry for the trouble.

mattheworiordan

I think this is a difficult PR to approve unfortunately because it depends on some other changes like the removal of the force attribute unfortunately

mattheworiordan · 2016-09-21T14:39:52Z

Source/ARTAuth.m

    [self storeParams:currentTokenParams];

    // Reuse or not reuse the current token
-    if (mergedOptions.force == NO && self.tokenDetails) {
+    if (currentAuthOptions.force == NO && self.tokenDetails) {


I think you should just remove force from this PR and perhaps bundle the change for RSA10d into this as this is only confusing the review of a PR that is in flux between old & new functionality

If I remove force property from AuthOptions object - it will break RSA10d & also RTC8 requirements, because both them needs force.
That’s why in my opinion it's impossible to combine both RSA10j and RSA10d changes in one PR.

Then combine more stuff into one PR. We asked for as small PRs as possible, but if it's not possible to deliver this without making other changes, then those changes make this PR as small as possible.

mattheworiordan · 2016-09-21T14:41:17Z

Source/ARTAuth.m

@@ -275,13 +275,13 @@ - (void)executeTokenRequest:(ARTTokenRequest *)tokenRequest callback:(void (^)(A

 - (void)authorise:(ARTTokenParams *)tokenParams options:(ARTAuthOptions *)authOptions callback:(void (^)(ARTTokenDetails *, NSError *))callback {
    BOOL requestNewToken = NO;
-    ARTAuthOptions *mergedOptions = [self mergeOptions:authOptions];
-    [self storeOptions:mergedOptions];
-    ARTTokenParams *currentTokenParams = [self mergeParams:tokenParams];


I believe timestamp and queryTime was not carried forward before, but now it appears they are given these objects are blindly copied? See http://docs.ably.io/client-lib-development-guide/features/#RSA10g

Got it, will improve.

mattheworiordan · 2016-09-21T14:43:12Z

Spec/Auth.swift

                            }
+                            expect(tokenDetails.clientId).to(equal(ExpectedTokenParams.clientId))
+                            expect(tokenDetails.issued!.dateByAddingTimeInterval(ExpectedTokenParams.ttl)).to(beCloseTo(tokenDetails.expires))
+                            expect(tokenDetails.capability).to(equal(ExpectedTokenParams.capability))


If your have removed the TTL and delay in the test, then how do you know the token is not the same token? You need to make sure that a new token has ben generated after calling authorise

mattheworiordan · 2016-09-21T14:50:44Z

Spec/Auth.swift

+                            guard let tokenDetails2 = tokenDetails2 else {
+                                XCTFail("TokenDetails2 is nil"); done(); return
+                            }
+                            expect(rest.auth.options.authCallback).to(beNil())


It would be more useful here to check that a documented API is overriden i.e. auth.options is undocumented, so instead why not check that the previous callback has not been used perhaps, or in the callback set a specific capability and ensure that capability has now not been used thus proving the callback has not been called.

ricardopereira · 2016-09-29T14:16:26Z

Squashed.

ricardopereira · 2016-09-29T20:57:20Z

@mattheworiordan PTAL

ricardopereira · 2016-09-29T22:32:01Z

@tcard PTAL

mattheworiordan

LGTM

mattheworiordan · 2016-10-03T07:46:21Z

Spec/Auth.swift

-                let tokenParams = ARTTokenParams(clientId: options.clientId)
-                let defaultTtl = tokenParams.ttl
-                let defaultCapability = tokenParams.capability
+                it("should supersede configured AuthOptions (using key) even if arguments are empty") {


This should say even argument objects are empty as saying arguments are empty makes it sound like if you don't pass in arguments as opposed to passing in empty objects

Done e4e97dd.

tcard · 2016-10-03T10:59:32Z

Spec/Auth.swift

+                        }
+                    }
+
+                    // Second time


What are we testing by doing it a second time? If it's to test that subsequent authorizations will use key = nil then you should pass options: nil the second time or force an authorization some other way, right?

Right. Done d069ed9.

tcard · 2016-10-03T11:18:48Z

Spec/Auth.swift

+
+                    // Repeat
+                    waitUntil(timeout: testTimeout) { done in
+                        rest.auth.authorise(nil, options: nil) { tokenDetails, error in


Like here, you're passing options: nil and I guess you should do the same in the test above.

tcard · 2016-10-03T11:33:46Z

Spec/Auth.swift

+                                XCTFail("TokenDetails is nil"); done(); return
+                            }
+                            expect(tokenDetails.clientId).to(beNil())
+                            expect(tokenDetails.issued).to(beCloseTo(serverDate, within: 1.0)) //1 Second


This doesn't really prove that queryTime was used, since it's really likely that the server time and the local time are within one second of each other. Maybe check that a call to /time was made? Really testing this would require that we mock the time locally, and I don't think that's worth it just for this.

Plus, anything could take longer that 1 second in the server for any reason and then serverDate would be outdated, especially given you're using it twice. I'd rather not introduce even more reasons for tests to randomly fail because we expect unguaranteed things from the server.

Yes, I'm having some failures on my machine because of that 1 second. I will mock the time. It will be necessary for another test about the server clock offset.

ricardopereira · 2016-10-04T11:23:17Z

@tcard PTAL

tcard

LGTM

tcard · 2016-10-04T19:13:27Z

Spec/NSObject+TestSuite.h

@@ -22,6 +22,9 @@ ART_ASSUME_NONNULL_BEGIN
 /// Get argument at index from the identified instance method.
 - (void)testSuite_getArgumentFrom:(SEL)selector atIndex:(NSInteger)index callback:(void (^)(id))callback;

+/// Change the returning value using a date from the identified instance method.
+- (id<AspectToken>)testSuite_returnValueFor:(SEL)selector withDate:(NSDate *)value;


EvgenyKarkan changed the base branch from master to RSA9h September 4, 2016 17:25

mattheworiordan reviewed Sep 7, 2016
View reviewed changes

EvgenyKarkan mentioned this pull request Sep 8, 2016

Rename authorise API #496

Closed

mattheworiordan reviewed Sep 9, 2016
View reviewed changes

tcard force-pushed the RSA10j branch from e47a015 to 5ba64ab Compare September 13, 2016 23:49

tcard changed the base branch from RSA9h to master September 13, 2016 23:50

tcard reviewed Sep 13, 2016
View reviewed changes

mattheworiordan requested changes Sep 21, 2016

View reviewed changes

ricardopereira force-pushed the RSA10j branch 2 times, most recently from b675c33 to 134c708 Compare September 29, 2016 14:15

mattheworiordan approved these changes Oct 3, 2016

View reviewed changes

tcard suggested changes Oct 3, 2016

View reviewed changes

ricardopereira force-pushed the RSA10j branch from f0683e4 to 02e219f Compare October 4, 2016 10:12

tcard approved these changes Oct 4, 2016

View reviewed changes

ricardopereira force-pushed the RSA10j branch from 02e219f to 92fcf40 Compare October 4, 2016 19:29

EvgenyKarkan and others added 4 commits October 4, 2016 20:31

RSA10j

582835c

Fix RSA10j

3e8f0d7

Auth.authorise: supersede arguments even if nil

38ba420

Test suite: force token to expire

cb83e7c

ricardopereira added 2 commits October 4, 2016 20:34

Test suite: change the return value of a method

6aa4e5f

Update old specs

1fb3a66

ricardopereira force-pushed the RSA10j branch from 92fcf40 to 1fb3a66 Compare October 4, 2016 20:02

ricardopereira merged commit ab283ec into master Oct 5, 2016

ricardopereira deleted the RSA10j branch October 5, 2016 08:01

ricardopereira mentioned this pull request Oct 12, 2016

Add reauth capability #452

Closed

@@ @@ -1903,6 +1903,32 @@ class Auth : QuickSpec { @@
                                       done()
                                   }
                               }

RSA10j #492

RSA10j #492

Conversation

EvgenyKarkan commented Sep 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard commented Sep 7, 2016

EvgenyKarkan commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgenyKarkan commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard commented Sep 14, 2016

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented Sep 29, 2016

ricardopereira commented Sep 29, 2016

ricardopereira commented Sep 29, 2016

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

tcard Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented Oct 4, 2016

tcard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard Oct 3, 2016 •

edited

Loading

tcard Oct 3, 2016 •

edited

Loading

tcard Oct 3, 2016 •

edited

Loading