Skip to content

Checking if error code is 403 and failing connection#672

Merged
QuintinWillison merged 6 commits intomainfrom
fix/620-fail-connection-on-forbiden
Jun 9, 2021
Merged

Checking if error code is 403 and failing connection#672
QuintinWillison merged 6 commits intomainfrom
fix/620-fail-connection-on-forbiden

Conversation

@martin-morek
Copy link
Copy Markdown
Contributor

@martin-morek martin-morek commented May 21, 2021

  • If server replies with 403 status code on authorisation attempt, connection state turns to Failed
  • Fixes #620

Comment thread lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated
@sacOO7
Copy link
Copy Markdown
Collaborator

sacOO7 commented May 22, 2021

Also, I strongly feel, we need to add couple of tests for the change made

@martin-morek
Copy link
Copy Markdown
Contributor Author

Also, I strongly feel, we need to add couple of tests for the change made @sacOO7

I definitely agree on this, however could you please point me what's the correct place to put them? I've seen there are already test classes but I cannot figurate out how to fail auth to get the correct error code.

Copy link
Copy Markdown
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

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

LGTM, given that I have little knowledge of the codebase and specs 😉

Copy link
Copy Markdown
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

It looks like the requirement isn't implemented - it states that the returned error needs to be wrapped in a new error whose statusCode is 403 and code is 80019.

Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeAuthTest.java
Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeAuthTest.java
Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeAuthTest.java
@sacOO7
Copy link
Copy Markdown
Collaborator

sacOO7 commented May 27, 2021

Also, add some details to the active PR, as in which issues it's fixing.
Adding one more reference implementation in JS

@Override
public void onConnectionStateChanged(ConnectionStateChange stateChange) {
assertEquals(stateChange.previous, ConnectionState.connected);
assertEquals(stateChange.reason.code, 80019);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should follow the right format, expected and actual values should be at the right places

@sacOO7 sacOO7 requested a review from paddybyers June 3, 2021 07:28
@jamienewcomb
Copy link
Copy Markdown
Member

jamienewcomb commented Jun 8, 2021

@QuintinWillison both @KacperKluka and @sacOO7 have approved this btw. But adding you if you wanted to check also

Edit - just seen Paddy's comment

@QuintinWillison QuintinWillison merged commit 65b65c4 into main Jun 9, 2021
@QuintinWillison QuintinWillison deleted the fix/620-fail-connection-on-forbiden branch June 9, 2021 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Fail connection immediately if authorize() called and 403 returned

6 participants