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.

Sign up for GitHub

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

Jump to bottom

RSA15a #89

Merged
merged 6 commits into from
Dec 29, 2015
Merged

RSA15a #89

merged 6 commits into from
Dec 29, 2015

Conversation

ricardopereira
Copy link
Contributor

No description provided.

mattheworiordan
mattheworiordan reviewed Nov 26, 2015
View reviewed changes
ablySpec/Auth.swift

waitUntil(timeout: 10) { done in
// Token
client.calculateAuthorization(ARTAuthMethod.Token) { token, error in
Copy link
Member

Choose a reason for hiding this comment

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

what is calculateAuthorization?

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 It is the REST method that checks the Auth method and completes the request with the Authorisation header: Basic key or Bearer token. When it is Token, it calls the Auth#authorise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name could be better. Maybe: calculateAuthorization -> buildAuthorisationHeader

Copy link
Member

Choose a reason for hiding this comment

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

prepareAuthorisationHeader?

@mattheworiordan
Copy link
Member

Looks good, however I don't think it's testing the error condition when the provided ClientOptions clientId does not match, which is really what the spec item is about.

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Could you clarify, please. Which error condition?

(RSA15a) Any clientId provided in ClientOptions must match any non null clientId value in TokenDetails or connectionDetails of the CONNECTED ProtocolMessage, where applicable

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Can you please merge this or review what is missing? I need the property that I added on the MockTransport.

@mattheworiordan
Copy link
Member

Could you clarify, please. Which error condition?

As a policy, we should always be testing the success and failure conditions. In this example, you are only testing the success condition which passes i.e. clientId does match the CONNECTED or TokenDetails clientId. Whilst this is needed, you have not covered off the other half of what this spec requires i.e. what happens when they are not compatible? When the clientId is not compatible, it should result in an error. We need to have a test for this.

Please bear this in mind in all future tests, we are always aiming to cover both success and failures for any requirement to prove it's implemented.

@ricardopereira
Copy link
Contributor Author

Done

mattheworiordan added a commit that referenced this pull request Dec 29, 2015
@mattheworiordan
Merge pull request #89 from ably/RSA15a
f71990d 
RSA15a
@mattheworiordan mattheworiordan merged commit f71990d into master Dec 29, 2015
@ricardopereira ricardopereira deleted the RSA15a branch January 5, 2016 13:43
maratal pushed a commit that referenced this pull request Jul 19, 2023
@SimonWoolf
Merge PR #89 'encryption' into source
4515bf0
maratal pushed a commit that referenced this pull request Jul 19, 2023
@QuintinWillison
Merge pull request #89 from ably/rest-request-with-version
a8efd5c 
Add mandatory version param for rest#request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ricardopereira @mattheworiordan