[ECO-4845] Fix wildcard clientId #418
Conversation
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
changed the title
lib/ably/auth.rb
Outdated
| def client_id_header(realtime=false) | ||
| if options[:client_id] && using_basic_auth? | ||
| if realtime | ||
| { 'clientId' => options[:client_id] } |
There was a problem hiding this comment.
For realtime connections the clientId is passed in the query params, not as a header - see RSA7e1
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, it's just a naming convention. I will try to make it more meaningful 👍
There was a problem hiding this comment.
As far as spec impl. is concerned, setting clientId for both realtime and rest follows exactly similar checks
spec- RSA7e
There was a problem hiding this comment.
maybe custom_client_id or external_client_id
There was a problem hiding this comment.
Yeah, perhaps we should just have a method which returns the client id to use in the request:
def client_id_for_request
if options[:client_id] && using_basic_auth?
return options[:client_id]
else
return nil
end
endand we update the existing extra_auth_headers method to call this, and update create_websocket_transport to call it?
There was a problem hiding this comment.
Cool, looks good to me — will leave to @AndyTWF to resolve if happy since it's his thread
|
@sacOO7 is this a bug that only affects the v2 branch? If not, then shouldn't the fix go into |
|
@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 |
Actually, this bug was caused by server side changes, so they have patched a fix that will work for ably-ruby <= 1.2.5 |
lib/ably/auth.rb
Outdated
| def client_id_header(realtime=false) | ||
| if options[:client_id] && using_basic_auth? | ||
| if realtime | ||
| { 'clientId' => options[:client_id] } |
There was a problem hiding this comment.
Yeah, perhaps we should just have a method which returns the client id to use in the request:
def client_id_for_request
if options[:client_id] && using_basic_auth?
return options[:client_id]
else
return nil
end
endand 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.
|
Thanks for the review |
4 participants
Fixed #414