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.

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

Refactor how we handle AblyAuth.ClientId #930

Open
marto83 opened this issue Oct 1, 2021 · 2 comments
Open

Refactor how we handle AblyAuth.ClientId #930

marto83 opened this issue Oct 1, 2021 · 2 comments

Comments

@marto83
Copy link
Contributor

marto83 commented Oct 1, 2021

Currently the ClientId just delegates to a number of other internal properties like ConnectionClientId, CurrentToken.ClientId, CurrentTokenParams.ClientId. Which was all good until we needed to know when the ClientId has changed. It will be better to update AblyAuth.ClientId to be a simple property that is changed when required. This way it will be much simpler to implement the notification handler.

┆Issue is synchronized with this Jira Task by Unito

@tomkirbygreen
Copy link
Contributor

@marto83 Can I close this issue now that #931 has landed on main?

@sacOO7
Copy link
Collaborator

sacOO7 commented Dec 19, 2021

Yeah, I think we should close this issue. @marto83 you can check again if the issue is resolved.

lawrence-forooghian added a commit to ably/docs that referenced this issue Jun 27, 2022
I might need to split this one into a separate issue depending on what I
can find out.

  clientId: String? // proxy for RSA7 TODO what? I can't see a spec point (nor does Rest have it) Auth has one
  // from Auth: clientId: String? // RSA7, RSC17, RSA12
// RSA7 doesn't even describe a property as far as I can tell, yet it's used for Auth's clientId property description in IDL too
* @(RSA7)@ @clientId@ and authenticated clients:
** @(RSA7d)@ If @clientId@ is provided in @ClientOptions@ and @RSa4@ indicates that token auth is to be used, the @clientId@ field in the @TokenParams@ (@tk2c@) should be set to that @clientId@ when requesting a token
** @(RSA7e)@ If @clientId@ is provided in @ClientOptions@ and @RSa4@ indicates that basic auth is to be used, then:
*** @(RSA7e1)@ For realtime clients, the connect request should include the @clientId@ as a querystring parameter, @clientId@
*** @(RSA7e2)@ For REST clients, all requests should include an @X-Ably-ClientId@ header with value set to the @clientId@, Base64 encoded
** @(RSA7a)@ A client is considered to be identified if a @clientId@ is implicit in either the connection or the authentication scheme; that is, is present in the current authentication token (with the exception of the wildcard @clientId@ @'*'@), is set by a header per @RSA7e2@, or is specified when initiating a realtime connection per @RSA7e1@. The following applies to identified clients:
*** @(RSA7a1)@ All operations (such as message publishing or presence) carried out by an identified client will have an implicit @clientId@. The Ably service automatically updates the @clientId@ attribute (when empty) for all @message@ and @PresenceMessage@ messages received from that client. Client libraries should therefore not explicitly set the @clientId@ field on messages published from an identified client
* @(RSC17)@ When instantiating a @restclient@, if a @clientId@ attribute is set in @ClientOptions@, then the @Auth#clientId@ attribute will contain the provided @clientId@
** @(RSA12)@ @Auth#clientId@ attribute is @null@ when:
*** @(RSA12a)@ The @clientId@ attribute of a @TokenRequest@ or @TokenDetails@ used for authentication is @null@, or @ConnectionDetails#clientId@ is @null@ following a connection to Ably. In this case, the @null@ value indicates that a @clientId@ identity may not be assumed by this client i.e. the client is anonymous for all operations
*** @(RSA12b)@ The client was instantiated without assigning a value for @ClientOptions#clientId@ (@null@), and the client has not yet authenticated or connected to Ably. In this case, the @null@ value indicates that the client has not yet been able to confirm its identity, and therefore may change and become identified following later authentication or establishment of a connection with Ably
  connection: Connection // RTC2
  request(
    String method,
    String path,
    Dict<String, String> params?,
    JsonObject | JsonArray body?,
    Dict<String, String> headers?
  ) => io HttpPaginatedResponse // RTC9
  stats(
    start: Time api-default epoch(),
    end: Time api-default now(),
    direction: .Backwards | .Forwards api-default .Backwards,
    limit: int api-default 100,
    unit: .Minute | .Hour | .Day | .Month api-default .Minute
  ) => io PaginatedResult<Stats> // Same as RestClient.stats, RTC5
  close() // RTC16
  connect() // RTC15
  time() => io Time // RTC6

// OK, does this RealtimeClient#clientId actually exist anywhere? And if so, does the corresponding Rest one exist?

// cocoa:
// yes on realtime, returns _rest.options.clientId, comes from the initial commit (git log -p -S '_clientId = options.clientId')
// yes on auth, returns
    if (_protocolClientId) {
        return _protocolClientId;
    }
    else if (self.tokenDetails && self.tokenDetails.clientId) {
        return self.tokenDetails.clientId;
    }
    else {
        return self.options.clientId;
    }
// no on rest

// js:
// yes (i.e. in .d.ts) on realtime, returns I have no idea, can't figure out where it's populated. asked about it in slack: https://ably-real-time.slack.com/archives/C017C5EPYG0/p1656369190712759
// yes on auth, returns erm, not sure, search for uses of _uncheckedSetClientId, which come from ConnectionManager and inside Auth
// no on rest

// java:
// realtime: I see no evidence of this property existing
// exists on auth although I haven't dived into how it works... see setClientId
// rest: I see no evidence of this property existing (but perhaps I missed it, given that I can't find it on realtime either)

// ruby:
// realtime: lib/ably/realtime/client.rb, def_delegators :auth, :client_id, :auth_options
  // https://ruby-doc.org/stdlib-2.5.1/libdoc/forwardable/rdoc/Forwardable.html#method-i-def_delegators
  // i.e. client_id is just forwarded to auth
// rest: lib/ably/rest/client.rb, as above w/ def_delegators to auth

// go:
// realtime:  I see no mention (at least in ably/realtime_client.go)
// auth: not looked into logic, but it has the property and there's also a updateClientID
// rest: not in ably/rest_client.go

// dotnet:
// realtime: src/IO.Ably.Shared/AblyRealtime.cs has `public string ClientId => Auth.ClientId;`
// auth:  src/IO.Ably.Shared/AblyAuth.cs,
        // TODO: Refactor how we hold the ClientId as per ably/ably-dotnet#930
        public string ClientId => ConnectionClientId
                                  ?? CurrentToken?.ClientId
                                  ?? CurrentTokenParams?.ClientId
                                  ?? Options.GetClientId();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants