Skip to content

Refactored RTN22, added missing details #259

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

Open
sacOO7 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactored RTN22, added missing details #259

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion textile/features.textile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ h3(#realtime-connection). Connection
** @(RTN7b)@ Every @ProtocolMessage@ that expects an @ACK@ or @NACK@ sent must contain a unique serially incrementing @msgSerial@ integer value starting at zero. The @msgSerial@ along with the @count@ for incoming @ACK@ and @NACK@ @ProtocolMessages@ indicates which messages succeeded or failed to be delivered
** @(RTN7c)@ If a connection enters the @SUSPENDED@, @CLOSED@ or @FAILED@ state, or if the connection state is lost, and an @ACK@ or @NACK@ has not yet been received for a message, the client should consider the delivery of those messages as failed, meaning their callback (or language equivalent) should be called with an error representing the reason for the state change, and they should be removed from any @RTN9a@ retry queue
** @(RTN7d)@ If the @queueMessages@ client option (@TO3g@) has been set to @false@, then when a connection enters the @DISCONNECTED@ state, any messages which have not yet been @ACK@d should be considered to have failed, with the same effect as in @RTN7c@
* @(RTN22)@ Ably can request that a connected client re-authenticates by sending the client an @AUTH@ @ProtocolMessage@. The client must then immediately start a new authentication process as described in "RTC8":#RTC8
* @(RTN22)@ Ably can request that a connected client re-authenticates by sending the client an @AUTH@ @ProtocolMessage@. The client must then immediately start a new authentication process as described in "RTC8":#RTC8. Ably sends @AUTH@ @ProtocolMessage@ 30 seconds before the token expires. Note: If the token has a ttl less than 30 seconds, e.g. 5 seconds, ably won't be able to send @AUTH@ @ProtocolMessage@.
Copy link
Member

@SimonWoolf SimonWoolf Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ably sends AUTH ProtocolMessage 30 seconds before the token expires. Note: If the token has a ttl less than 30 seconds, e.g. 5 seconds, ably won't be able to send AUTH ProtocolMessage

The spec specifies SDK behaviour. The added sentences don't have any requirements, they're background information. What are SDK devs expected to do differently as a result of this information?

Like: currently, the server does not guarantee that it sends an AUTH exactly 30 seconds before expiry, nor does it guarantee that it won't send one if you connect with a <30s token. Both things are at the server's discretion, and might change at any point. By putting this in the spec, we are saying that SDK devs should be able to rely on the current behaviour. Is that necessary? If so, for what purpose are they relying on it?

Copy link
Collaborator Author

@sacOO7 sacOO7 Dec 18, 2024

Choose a reason for hiding this comment

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

  • I believe this won't affect core SDKs, but currently we added check as a part of laravel token refresh PR
  • I had a hunch about value change, so the PR. Btw, few questions regarding this
  1. How frequently do we change this value? or when was the last time value was changed ?
  2. Currently, laravel-echo client is restricted to use protocol 2 or ably-js@1.x, can we still change the value from server side?

Copy link
Collaborator Author

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If value can change in the future ( with certain max limit, say 1 minute ), I can update PR accordingly

Copy link
Member

@SimonWoolf SimonWoolf Dec 18, 2024

Choose a reason for hiding this comment

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

How frequently do we change this value? or when was the last time value was changed ?

It's never changed. But that's not really the point; the server has behaviours that it guarantees as part of its protocol and behaviours that are at its discretion, and we should only move something from the latter category to the former if there's a good reason to do so.

I'd like to understand why laravel-broadcaster needs to know how long before expiry it'll get an AUTH. I've looked at the linked PR but I don't have the context to understand what it's doing or why without that it's requesting a token without all the required capabilities. Could you explain?

Copy link
Collaborator Author

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay current laravel impl. goes as follows

  1. Each requested token has default 1 hr expiry. Each channel capability can be configured at server side using configure channel capability.
  2. For each private/presence channel join, internally new jwt token is requested. As a part of request, it sends previous token. Server validates the previous token, checks if it is valid, and has previous private/presence channels. If it has, token is upgraded with the latest requested channel capabilities while maintaining old channels. While token is upgraded, we set new token expiry same as old token. So, after token expires ( default 1 hr expiry ), the client loses access to all channels along with their capabilities.
  3. As a part of new token request after token expiry, although old token is provided ( since it's expired ), we return token with only public channels access. This means after token expiry, each channel will detach and will re-request token. This will cause presence enter/leave as customer explained -> https://ably-real-time.slack.com/archives/CURL4U2FP/p1733135454672739

Copy link
Collaborator Author

@sacOO7 sacOO7 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, about statement Ably sends @AUTH@ @ProtocolMessage@ 30 seconds before the token expires. Note: If the token has a ttl less than 30 seconds, e.g. 5 seconds, ably won't be able to send @AUTH@ @ProtocolMessage@., This is useful for testing purposes ( mainly integration tests ).
But if you feel, the value can change in the future, we can skip the change or mention that value can change in the future, WDYT

Copy link
Member

@SimonWoolf SimonWoolf Dec 18, 2024

Choose a reason for hiding this comment

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

We can upgrade token expiry to new value every time user joins a new channel, but then there won't be any point of having token expiry

It seems to me that this claim contradicts the change you've made in the PR.

If the point of having tokens expire is that the customer, via their auth server, must approve (i.e. have the ability to disapprove) every user's right to be in each private channel at least once per hour (or whatever period), then surely having the auth server automatically re-approve all permissions if the user's token is about to expire (without asking the customer) defeats that point.

(And if we don't believe that, if we think it's acceptable for the auth server to automatically re-issue the token with all its previous permissions if it's near expiry, then why not extend the expiry every time a new channel is added? But I don't think we do believe this; as you say that would defeat the point of token expiry).

Should the broadcaster not re-ask the customer to confirm permissions (using verifyUserCanAccessChannel) for each non-public channel in the token if the token is near expiry (which would be something like, on any auth request with < 10 minutes to go, so including channel access requests made towards the end, not just 30 seconds), and then it can extend the expiry time without breaking the auth model?

Copy link
Collaborator Author

@sacOO7 sacOO7 Dec 18, 2024

Choose a reason for hiding this comment

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

Okay, I will give a thought about this and will get back 👍

Copy link
Collaborator Author

@sacOO7 sacOO7 Jan 2, 2025

Choose a reason for hiding this comment

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

It seems sensible to verify for each non-public channel if the token is near expiry, before refreshing to new token with upgraded expiry. Will update PR accordingly in the future 👍

Copy link
Collaborator Author

@sacOO7 sacOO7 Jan 2, 2025

Choose a reason for hiding this comment

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

Added TODO for the same ably/laravel-broadcaster#56 (comment)

** @(RTN22a)@ Ably reserves the right to forcibly disconnect a client that does not re-authenticate within an acceptable period of time, or at any time the token is deemed no longer valid. A client is forcibly disconnected following a @DISCONNECTED@ message containing an error code in the range @40140 <= code < 40150@. This will in effect force the client to re-authenticate and resume the connection immediately, see "RTN15h":#RTN15h
* @(RTN8)@ @Connection#id@ attribute:
** @(RTN8a)@ Is unset until connected
Expand Down
Loading