Add namespace option to BrowserOAuthClient #3516
Open
+39
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ScottPolhemus
force-pushed
the
browser-oauth-namespace
branch
from
7541842 to
693fff1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, this is my first atproto PR so please let me know if there's anything I can do to make it easier to review!
What problem is this trying to solve?
Currently, if you attempt to host multiple AppViews on the same domain using the browser-based OAuth client they will share the same localStorage keys and indexedDB database name. This can cause problems when, for instance, one app tries to refresh a token that was issued on behalf of the other app's client ID. Ideally we could host multiple apps on different paths of the same domain and each would use their own independent authentication values.
I recognize that this may be an uncommon use-case, but I personally have my blog's "AppView" (with an associated CMS interface using ATProto+OAuth) and a separate ATProto-based flashcard studying tool both hosted as static-generated sites on the same domain using GitHub Pages.
Could I host one or both apps under a different domain to work around this issue? Sure, and feel free to tell me to take a hike if this is not a common enough use-case to warrant the team's attention and I should work around it in another way. I first wanted to take a stab at providing a solution for it within the official library rather than seeking alternative OAuth tooling or adjusting my hosting setup.
My solution
This PR adds a
namespaceoption toBrowserOAuthClientallowing users to specify the namespace used for browser storage. When specified, it will override the default namespace for localStorage keys and the default indexedDb database name as well as the broadcast channels and popup state prefix keys.I chose this approach to avoid unexpectedly breaking existing functionality. If you do not specify a custom namespace, the behavior is identical to the current version. A more "elegant" solution might be to automatically generate per-app namespaces based on the app's Client ID, but this could unexpectedly break existing sessions in some apps and would thus require a major version bump which seems excessive for this small of an issue.
Thanks for taking a look 🙂