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

Remove basic auth #31

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Remove basic auth #31

merged 8 commits into from
Mar 6, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Mar 4, 2024

No description provided.

Copy link

render bot commented Mar 4, 2024

Copy link

render bot commented Mar 4, 2024

@foysalit foysalit requested review from devinivy and dholms March 4, 2024 21:33
Comment on lines 69 to 74
const { data } = await client.api.com.atproto.identity.resolveHandle(
{
handle: id,
},
{ headers: client.proxyHeaders() },
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to proxy this one— ozone doesn't have an API endpoint for com.atproto.identity.resolveHandle.

Comment on lines +12 to +17
const { data } = await client.api.app.bsky.feed.getFeedGenerator(
{
feed: uri,
},
{ headers: client.proxyHeaders() },
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 24 to 27
await clientManager.api.com.atproto.admin.deleteCommunicationTemplate(
{
id: templateId,
},
{
headers: clientManager.adminHeaders(),
encoding: 'application/json',
},
{ id: templateId },
{ encoding: 'application/json' },
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we lost the headers on this one.

Comment on lines +12 to +18
const { data } = await client.api.app.bsky.graph.getList(
{
list: uri,
limit: 1,
},
{ headers: client.proxyHeaders() },
)
Copy link
Collaborator

@devinivy devinivy Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +564 to +569
const { data } = await client.api.app.bsky.actor.getProfiles(
{
actors,
},
{ headers: client.proxyHeaders() },
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to sync on the backend side of things, but 👏 this is awesome!

lib/client.ts Outdated
const adminToken = override ?? this.session.adminToken
return { authorization: `Basic ${btoa(`admin:${adminToken}`)}` }
proxyHeaders(override?: string): Record<string, string> {
const proxy = override ?? process.env.ATPROTO_PROXY
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this environment variable need to be prefixed with NEXT_PUBLIC_ in order to make it into the build?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought, perhaps we should make the env var more descriptive about what it is. I believe the idea here is to specify the ozone DID and service id. So maybe it should be something like OZONE_SERVICE_DID. Any thoughts @dholms ?

lib/client.ts Outdated Show resolved Hide resolved
Comment on lines -61 to -65
// Check validity of admin token
await agent.api.com.atproto.admin.getRepo(
{ did: login.did },
{ headers: this.adminHeaders(adminToken) },
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually might want to keep a check similar to this: your PDS is going to let you login, but the ozone service may not let you use it due to ACLs.

@devinivy devinivy merged commit 58797d5 into main Mar 6, 2024
3 checks passed
@devinivy devinivy deleted the remove-basic-auth branch March 6, 2024 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants