[ECO-4845] Fix wildcard clientId by sacOO7 · Pull Request #418 · ably/ably-ruby

sacOO7 · 2024-06-25T10:28:47Z

Added client_id_for_request method
Added method client_id_for_request_sync to realtime auth
Annotated implementation with right spec

1. Renamed rest auth extra_auth_headers to client_id_header with both rest/realtime specific impl 2. Added method client_id_header_sync to realtime auth 3. Annotated implementation with right spec

AndyTWF · 2024-06-25T10:31:51Z

+ def client_id_header(realtime=false)
+ if options[:client_id] && using_basic_auth?
+ if realtime
+ { 'clientId' => options[:client_id] }


For realtime connections the clientId is passed in the query params, not as a header - see RSA7e1

I think by putting the realtime setting in a method that explicitly mentions header (even if its later put in the right place by subsequent code) is going to make things confusing

Yeah, it's just a naming convention. I will try to make it more meaningful 👍

As far as spec impl. is concerned, setting clientId for both realtime and rest follows exactly similar checks
spec- RSA7e

Updated -> bb4b71f

maybe custom_client_id or external_client_id

updated -> 0f49ede

Yeah, perhaps we should just have a method which returns the client id to use in the request:
defclient_id_for_requestifoptions[:client_id] && using_basic_auth?returnoptions[:client_id]elsereturnnilendend
and we update the existing extra_auth_headers method to call this, and update create_websocket_transport to call it?

Fixed 1ff7d6b

Cool, looks good to me — will leave to @AndyTWF to resolve if happy since it's his thread

… members

lawrence-forooghian · 2024-07-02T12:28:00Z

@sacOO7 is this a bug that only affects the v2 branch? If not, then shouldn't the fix go into main so that we can release it ASAP?

ttypic · 2024-07-02T12:31:03Z

@lawrence-forooghian there is no rush, this problem have been for a long time. Workaround on realtime side implemented for all version <= 1.2.5

sacOO7 · 2024-07-02T12:40:38Z

@sacOO7 is this a bug that only affects the v2 branch? If not, then shouldn't the fix go into main so that we can release it ASAP?

Actually, this bug was caused by server side changes, so they have patched a fix that will work for ably-ruby <= 1.2.5
Link to internal conversation for more information -> https://ably-real-time.slack.com/archives/C8SPU4589/p1718885475429199

lawrence-forooghian · 2024-07-02T12:54:03Z

+ def client_id_header(realtime=false)
+ if options[:client_id] && using_basic_auth?
+ if realtime
+ { 'clientId' => options[:client_id] }


Yeah, perhaps we should just have a method which returns the client id to use in the request:
defclient_id_for_requestifoptions[:client_id] && using_basic_auth?returnoptions[:client_id]elsereturnnilendend
and we update the existing extra_auth_headers method to call this, and update create_websocket_transport to call it?

…ce local members" This reverts commit c10e485.

ttypic

LGTM!

sacOO7 · 2024-07-04T18:30:11Z

Thanks for the review

[ECO-4845] refactored rest/realtime auth
f4d7f2b
1. Renamed rest auth extra_auth_headers to client_id_header with both rest/realtime specific impl 2. Added method client_id_header_sync to realtime auth 3. Annotated implementation with right spec

sacOO7 requested review from AndyTWF, SimonWoolf and lawrence-forooghian June 25, 2024 10:28

github-actionsBottemporarily deployed to staging/pull/418/features June 25, 2024 10:29 Inactive

sacOO7 changed the title ~~[ECO-4845] Refactored rest/realtime auth~~[ECO-4845] Fix wildcard clientId Jun 25, 2024

github-actionsBottemporarily deployed to staging/pull/418/docs June 25, 2024 10:29 Inactive

AndyTWF reviewed Jun 25, 2024
View reviewed changes

Fixed naming convention for client_id_header as per review comment
bb4b71f

github-actionsBottemporarily deployed to staging/pull/418/features June 25, 2024 10:43 Inactive

github-actionsBottemporarily deployed to staging/pull/418/docs June 25, 2024 10:43 Inactive

sacOO7 requested a review from AndyTWF June 25, 2024 10:44

Removed unncessary use of auth clientId while entering presence local…
c10e485
… members

github-actionsBottemporarily deployed to staging/pull/418/features June 25, 2024 10:53 Inactive

github-actionsBottemporarily deployed to staging/pull/418/docs June 25, 2024 10:54 Inactive

renamed auth client_id_header to external_client_id
0f49ede

github-actionsBottemporarily deployed to staging/pull/418/features June 25, 2024 10:59 Inactive

github-actionsBottemporarily deployed to staging/pull/418/docs June 25, 2024 10:59 Inactive

sacOO7 requested review from ttypic and removed request for ttypic July 2, 2024 10:21

lawrence-forooghian reviewed Jul 2, 2024
View reviewed changes

Base automatically changed from feature/protocol-2-presence to feature/integration-protocol-2July 3, 2024 11:50

sacOO7 added 2 commits July 3, 2024 17:35

Added separate method client_id_for_request that returns client_id
1ff7d6b

Revert "Removed unncessary use of auth clientId while entering presen…
91e5f29
…ce local members" This reverts commit c10e485.

github-actionsBotdeployed to staging/pull/418/features July 3, 2024 12:08 View deployment

github-actionsBotdeployed to staging/pull/418/docs July 3, 2024 12:08 View deployment

sacOO7 requested a review from lawrence-forooghian July 3, 2024 12:09

lawrence-forooghian approved these changes Jul 4, 2024
View reviewed changes

ttypic approved these changes Jul 4, 2024
View reviewed changes

sacOO7 merged commit 08877bf into feature/integration-protocol-2Jul 4, 2024

sacOO7 deleted the fix/wildcard-clientId-request branch July 4, 2024 18:30

sacOO7 mentioned this pull request Jul 4, 2024
Failing Test - retrieves two pages of messages before channel was attached #414
Closed

Uh oh!

Conversation

sacOO7 commented Jun 25, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sacOO7Jun 25, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lawrence-forooghian commented Jul 2, 2024

Uh oh!

ttypic commented Jul 2, 2024

Uh oh!

sacOO7 commented Jul 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttypic left a comment

Choose a reason for hiding this comment

Uh oh!

sacOO7 commented Jul 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

sacOO7 commented Jun 25, 2024•
edited
Loading

sacOO7Jun 25, 2024•
edited
Loading

sacOO7 commented Jul 2, 2024•
edited
Loading