Implements multi-user accounts and Refresh Expired Auth Tokens

[hierophantos]

This feature commit introduces some major changes.

1. Account Management:

   • Adds accounts.clj, which implements multiuser accounts in a global atom, accounts

   • clarifies the use of the abstraction session, which is the return object from ATProto createSession from the conflation with martian.core.Martian instance

   • an account is stored as a keyword of user handle in accounts, and a map structure of :session :m-spec and a :config which holds base-url and openapi source string

   • pretty much all functions now take a simple handle as a parameter, from which the relevant information is retrieved or populated via the global accounts atom

2. Refresh Expired Auth Tokens:
   • Reason for needing to store ATProto Session, is to hold onto the :refreshJwt, which is distinct from the :accessJwt, to refresh session on expired :accessJwt tokens
   • stores all refresh token Timers in accounts atom :refresh-futures (should change name...), maybe :refresh-timers, yeah
3. Auth:
   • is currently handled by reading in ./auth.edn in the project root directory and expects a structure like:

{:handle1 {:username "handle1"
           :account-password __password1__
           :base-url "https://bsky.social"}
 :handle2 {:username "handle2"
           :account-password __password2__
           :base-url "https://custom-pds.xyz"}...}

• auth is a bit of a WIP... happy for suggestions, recommendations on this

4. Client changes:
   • Since accounts.clj holds most of the logic for token-sessions, etc., client is now a refactored call
   • also added another functions response-for that returns the :body of call
   • if no handle is provided, it will use the :default-account in accounts
   • if accessJwt is expired, will automatically call refresh-session! and retry
5. Other Changes:
   • Moved the spec definitions to .../atproto/specs/core.clj - could be expanded upon in the future
   • adds jwt.clj for parsing session token, in order to know when to schedule refresh

Resolves: [https://github.com//issues/6] [https://github.com//pull/8]

[goshatch]

Hi @hierophantos, thank you so much for the PR!

As a first step, could I please ask you to update the documentation (well, the README file, really), with updated usage?

Regarding auth, just as an idea, we could get some inspiration from the various emacs packages that need auth to be stored, and setup an overridable function that would retrieve the auth? It could come from an auth.edn file by default, but overriding the function would enable someone to get the auth from a password manager like pass or 1password, or some other way.

[levand]

Hey @hierophantos , thanks so much for your interest and contribution!

I think I'm missing a little bit of context regarding the motivation of this rewrite. @goshatch and I had collaborated on the original design and I had thought it already satisfied the concerns addressed here (with token refresh handled by PR #8).

In the original code, to work with multiple accounts, you'd just call init multiple times. Then the caller can manage those session objects however they choose. It's true this results in multiple Martian session objects, but they are fairly lightweight. It also permits having any configuration option be different for different sessions (e.g base url), which could be desirable in some cases. Environment variables aren't required, they are just a fallback in case a value is not provided in the specified configuration (this is the same way libraries like the AWS SDK work).

At a high level, I'm a bit concerned with the level of state that this change introduces to the library. Most of the methods alter some persistent state, meaning that users of the library need to comprehend not just the function signatures but the way the library manages state in general. As much as possible, libraries (even moreso than regular code) should be stateless in Clojure.

I also think it is a mistake to require config to be loaded from a specific file. There are many deployment situations where that is undesirable or impossible, and in general I'd prefer not to have a library make that choice for me... configuration values should be passed in. The client can load them from a file if they want. Even the env var fallback is just a convenience: fundamentally, libraries should be configured by the caller.

I like the change to actually inspect the timeout on the token, although we still need to handle the "fail then reauth" pattern since clock skew is real. Having a global stateful timer for the refresh is unnecessary, however: expiration can be checked prior to each call, keeping the library itself stateless.

[goshatch]

Thank you @levand 🙏

Hi @hierophantos! First of all, I want to apologise for the awkward communication on this topic: this is my first time running an open source project with collaborators and I'm still finding my feet.

I had a chance to review the code, and I would tend to agree with @levand's points, especially when it comes to letting users of the SDK to set up state of their application the way that works for them, rather than adding state to our library.

For the next steps, I would propose the following:

1. Close this PR in favour of Automatically refresh expired auth tokens #8, and cherry pick some of your changes (especially around the ExpiredToken error) for that PR.
2. Add an example namespace to the examples/ directory that would demonstrate how to use the HTTP client with multiple accounts and some methods for storing auth.

I would welcome your feedback and improvements on both of these, and again, apologies for any awkwardness in handling this.

[hierophantos]

I think I'm missing a little bit of context regarding the motivation of this rewrite. @goshatch and I had collaborated on the original design and I had thought it already satisfied the concerns addressed here (with token refresh handled by PR #8).

One motivating factor was to clarify the abstraction around what is being called session, and how it is conflated with the martian.core.Martian instance object. This rewrite decomplects the two, calling the latter, m-spec. The other reason for my code was that I thought multiaccount function and token refresh was wanted from the issues, and wasn't aware there was code addressing this already.

In the original code, to work with multiple accounts, you'd just call init multiple times. Then the caller can manage those session objects however they choose. It's true this results in multiple Martian session objects, but they are fairly lightweight. It also permits having any configuration option be different for different sessions (e.g base url), which could be desirable in some cases.

In my rewrite, things work similarly. User calls (login! "user-handle-here.bsky.social"), resulting in the global atom accounts to be written to under their handle as a keyword and a map of maps for :session, :m-spec, and :config. This still allows for arbitrary configuration. I do make use of :base-url, for instance, from my personal PDS, so keeping things open for future configuration is top of mind.
accounts.handle.session
accounts.handle.m-spec
accounts.handle.config

I've tried to document this throughout the codebase, and even included spec definitions that show this structure, and can be later incorporated in further validation and testing.

At a high level, I'm a bit concerned with the level of state that this change introduces to the library. Most of the methods alter some persistent state, meaning that users of the library need to comprehend not just the function signatures but the way the library manages state in general. As much as possible, libraries (even moreso than regular code) should be stateless in Clojure.

For people simply making API calls, (login! handle) and then (call handle endpoint params) is the flow. They don't have to think of or worry what session is. It also implements a default user, so (call endpoint params) is all they'd need for single-user authenticated calls.

I also think it is a mistake to require config to be loaded from a specific file. There are many deployment situations where that is undesirable or impossible, and in general I'd prefer not to have a library make that choice for me... configuration values should be passed in. The client can load them from a file if they want. Even the env var fallback is just a convenience: fundamentally, libraries should be configured by the caller.

I agree. I call this out in my comments concerning Auth in general. I'm conceptualizing a namespace atproto.auth that provides some more general approaches to this, but I don't have concrete direction for that currently.

I like the change to actually inspect the timeout on the token, although we still need to handle the "fail then reauth" pattern since clock skew is real. Having a global stateful timer for the refresh is unnecessary, however: expiration can be checked prior to each call, keeping the library itself stateless.

Indeed. The changes to client/call does implement a reauth on failure. I do agree, keeping ahold of the timer is probably unnecessary. There are also a few other functions in accounts.clj that are unnecessary/could be trimmed, that are mostly dangling artifacts from when I was developing.

[goshatch]

Thank you for the discussion, everyone. I'm going to close this PR, as automatic token refresh has already been implemented in #6, and an issue has been created about providing some example code for multi-account auth (#10)