-
Notifications
You must be signed in to change notification settings - Fork 103
feat: atproto oauth #273
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
base: main
Are you sure you want to change the base?
feat: atproto oauth #273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
just a couple of minor comments but looks good to me, pretty straightforward! nice work 🎉 |
jonathanyeong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
| }) | ||
|
|
||
| const response = await fetch( | ||
| `${SLINGSHOT_ENDPOINT}/xrpc/com.bad-example.identity.resolveMiniDoc?identifier=${agent.did}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my knowledge, why does the endpoint contain bad-example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fig lol. XRPC's need a nsid to be complete and they used their domain for it
fatfingers23
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! all the atproto stuff is atprotoing
| 'api.bitbucket.org', // Bitbucket API | ||
| 'codeberg.org', // Codeberg (Gitea-based) | ||
| 'gitee.com', // Gitee API | ||
| //microcosm endpoints for atproto data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
| //microcosm endpoints for atproto data | |
| // microcosm endpoints for atproto data |
| export const OAUTH_SESSION_CACHE_STORAGE_BASE = 'oauth-atproto-session' | ||
|
|
||
| export class OAuthSessionStore implements NodeSavedSessionStore { | ||
| //TODO not sure if we will support multi accounts, but if we do in the future will need to change this around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
| //TODO not sure if we will support multi accounts, but if we do in the future will need to change this around | |
| // TODO: not sure if we will support multi accounts, but if we do in the future will need to change this around |
| } | ||
| deleteCookie(this.event, this.cookieKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a factory at the bottom to clean up the API a bit?
export const useOAuthStorage = (event: H3Event) => {
return {
states: new OAuthStateStore(event),
sessions: new OAuthSessionStore(event),
}
}
That way when it gets invoked elsewhere, like you do above in server/api/auth/atproto.get.ts, it could be as simple as const storage = useOAuthStorage(event).
| if (!process.env.NUXT_SESSION_PASSWORD) { | ||
| throw createError({ | ||
| status: 500, | ||
| message: 'NUXT_SESSION_PASSWORD not set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Extract to constants file
| message: 'NUXT_SESSION_PASSWORD not set', | |
| message: UNSET_NUXT_SESSION_PASSWORD, |
| if (!process.env.NUXT_SESSION_PASSWORD) { | ||
| throw createError({ | ||
| status: 500, | ||
| message: 'NUXT_SESSION_PASSWORD not set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Extract to constants file
| message: 'NUXT_SESSION_PASSWORD not set', | |
| message: UNSET_NUXT_SESSION_PASSWORD, |
| if (!process.env.NUXT_SESSION_PASSWORD) { | ||
| throw createError({ | ||
| status: 500, | ||
| message: 'NUXT_SESSION_PASSWORD not set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Extract to constants file
| message: 'NUXT_SESSION_PASSWORD not set', | |
| message: UNSET_NUXT_SESSION_PASSWORD, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add validation here since we're stepping outside of the system, the query params and a third-party API. Casting these with as could hide potential runtime crashes if for whatever reason the PDS or Slingshot return unexpected shapes.
AuthRequestSchemaorAuthQuerySchema- For the validategetQuery(event)- MiniDocSchema - For the fetch response from the Slingshot endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, this will be exposed to the public via the ATProto network? Would be nice to validate the output with a schema here as well. Just to guard against breakage for the OAuth handshake with an invalid URL or missing field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add a validation schema here we get the benefit of not having to perform repeat manual checks for NUXT_SESSION_PASSWORD and having a SessionDataSchema parse the session.data ensures the FE gets a reliable shape. Bonus is we won't accidentally leak any internal session metadata added down the line.
|
we can ignore the provenance warnings btw |
Kai-ros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my above comments on validation. Regardless, great work.
Implements AT Protocol OAuth using
@atproto/apiand@atproto/oauth-client-node. The form allows users to login with a handle from the app, create an account through selfhosted.social, or go through Bluesky.npmx-oauth.mp4
Notes:
NUXT_SESSION_PASSWORDenv variable to encrypt cookies