🚧 OAuth - Client SDK

[matthieusieben]

Why

From a conceptual point of view, An XRPC client should not expose which "service" will be triggered under the hood when the call() method is invoked. All is should expose is a simple call(method, ...) signature.

This separation of concerns is especially useful in Bluesky's architecture since in that particular case, two "services" are actually needed to properly route calls: The entryway (that provides credentials) and the PDS (that handles the actual HTTP request).

This separation of concerns becomes even more useful when we introduce the support for various authentication strategies (e.g. OAuth), which may introduce special behaviors (e.g. automatic session refresh, sixgning of HTTP requests, DPoP proofs).

In its current form, the Client and ServiceClient classes from @atproto/xrpc do expose the uri being triggered. This yields to users of that class having to know the internals of the XRPC Client and requires them to manually change that uri when needed:

https://github.com/bluesky-social/social-app/blob/1e484c6318c0f1a2fa46c028d53b92d817293d11/src/state/session/index.tsx#L316

This PR inverts this logic by allowing a specialized class (XrpcDispatcher) to decide how to actually dispatch the HTTP call. That class can not only "decide" which service endpoint will be triggered but it can also perform additional tasks such as handling authentication. In particular, it can inject authorization headers and handle http errors in order to retry.

This change was motivated by the fact that session management in case of OAuth is radically different to what exists now.

Details

A dispatcher has a signature:

interface Dispatcher {
  dispatch(path: string, init: RequestInit): Promise<Response>
}

A Dispatcher is responsible to:

1. build the full URL
2. make the HTTP call (typically using fetch())
3. handle extra stuff (auth headers, session refresh, retries, etc.)

The dispatcher is used like so:

declare const dispatcher: Dispatcher // Define this somehow

const client = new XrpcClient(dispatcher)
await client.call('com.atproto.server....', params)

The default XrpcDispatcher can be used to perform basic calls:

const dispatcher = new XrpcDispatcher({
  service: 'http://example.com',
  headers: { authorization: 'Bearer 123' }
})

const client = new XrpcClient(dispatcher)

Note that the XrpcClient constructor will instantiate a default XrpcDispatcher if a dispatcher was not passed are argument:

const client = new XrpcClient({ service: "https://example.com" })

A more advanced use case would typically inject a dispatch function:

let service = "https://example.com"
const client = new XrpcClient(async (url: string, init: RequestInit): Promise<Request> => {
  // Retry any fetch() error
  const fullUrl = new URL(url, service)
  try {
    return await fetch(fullUrl, init)
  } catch (err) {
    return await fetch(fullUrl, init)
  }
})

The AtpDispatcher extends XrpcDispatcher by adding:

• getDid(): get the did of the currently logged in user
• getServiceUrl(): get the entry way url

This abstraction is extended by either:

• OAuthDispatcher that will implement this interface using OAuth endpoints
• SessionDispatcher where were moved the following items
  • Making the fetch() call is performed here
  • Building the endpoint url (pdrUrl || serviceUrl)
  • Adds the Authorization header
  • Perform session management (all the methods previously defined on AtpAgent related to session management): createAccount, login, resumeSession & refreshSession + emitting of session changed related events (AtpPersistSessionHandler). The logic here is exactly was was present in AtpAgent.

Because the OAuthDispatcher will no longer have the ability to login(), createAccount(), etc. in the same fashion, those methods are no longer exposed on the AtpAgent itself.

The impact of this is that now, in order to use the conventional session management methods, one must use the SessionDispatcher instead.

Before

  const agent = new BskyAgent({service})
  await agent.login({identifier, password, authFactorToken})

After

  const dispatcher = new SessionDispatcher({service})
  await dispatcher.login({identifier, password, authFactorToken})
  const agent = new BskyAgent(dispatcher)

Future: The end goal is to replace the SessionDispatcher by an oauthClient like so:

  const agent = new BskyAgent(oauthClient)

[devinivy]

While our xrpc client will send a content-type header, I'm not sure it's formally a requirement, especially for any endpoint that receives application/octet-stream (since servers may interpret a missing content type in this way). We want to ensure we're compatible with other implementations, and I think that means sticking close to HTTP in any cases of ambiguity.

Though I'm also curious, was there something wrong with the existing implementation that required this change? The only thing that stands out to me is that it's possible we should be checking for chunked encoding as opposed to any transfer encoding.

[matthieusieben]

It is currently not possible to upload an empty text file. I added a test for that.

I also changed this again since you reviewed to work more like it used to. I "just" added the following condition that will also considered as having a body:

// e.g. an empty txt file
if (req.headers['content-length'] === "0" && req.headers['content-length']) return true

The rest of the diff in this function is purely aestetical.

[devinivy]

Looking good 👍

[pfrazee]

Okay this is on a good path. My main concern at this stage is that there's a lot of complexity that you inherited. You made very strong choices within that -- but we still have a pretty complicated class hierarchy:

The "XRPC" concept being separated may not be doing us any real favors at this stage. I wonder if we can compress it along these lines:

Good	Better?

This is a significant amount of additional work so I think we should talk about this. I don't want to unduly hold up OAuth more than I already have.

[matthieusieben]

reworked in #2483