commits
Required according to rfc7523
Addresses #1152
This can be tested by initiating a login with the demo client app, but
hitting "cancel" on the AS login page (or by refusing to grant the
requested scopes).
The root cause was that this package was assuming that query and
procedure "params" were required, when they are actually optional.
This corrects them to be optional (pointer type), and skips validation
when nil.
Also adds "minimal" example schemas as regression tests.
Fixes: https://github.com/bluesky-social/indigo/issues/1153
I should probably add parsing helpers to the atproto/syntax package for
MIME type glob patterns, and for "service refs".
This isn't super rigorous yet, and it doesn't support pass-through of
extra fields on `permission` types. But it would catch early/initial
syntax issues, and could be used with `goat` for publishing and
resolving lexicons (eg, basic devex for permission-sets feature).
This is a breaking change, and could impact database schemas or even
stored data for already-deployed code relying on this package.
On the other hand, it makes things consistent, and I think easier to
work with.
I'll try to do a quick scan for projects already building on this (new)
package.
I believe that we need to set the accountDID to ident.DID so we can set
info.DID later on if we are not given an auth server url
Accounts sometimes don't have an entry in their account_repo extension
table. This change handles that case for listRepos by including them in
the response but with empty strings for the "Rev" and "Head" fields.
...which probably isn't ultimately correct, but is at least more usable
for clients than missing an entire page of repos because one of them was
affected by this.
placeholder improvement directly related to
https://github.com/bluesky-social/indigo/issues/1143
basically the same as #1142 but that PR only addressed getRepoStatus
---
for what it's worth, the sql join `TODO` suggested in the code here
would have hidden (fixed?) this problem by omitting the affected
accounts, assuming inner-join
Accounts sometimes don't have an entry in their account_repo extension table. This change handles that case for listRepos by including them in the response but with empty strings for the "Rev" and "Head" fields.
...which probably isn't ultimately correct, but is at least more usable for clients than missing an entire page of repos because one of them was affected by this.
placeholder improvement directly related to https://github.com/bluesky-social/indigo/issues/1143
(also for what it's worth, the sql join `TODO` would have hidden (fixed?) this problem by omitting the affected accounts (assuming an inner-join))
this change just makes sure `repo` is not nil before we try to pull a
`Rev` out of it.
assigning the Rev from the repo follows what's *almost* a boilerplate
golang error handler, but is sneakily not handling
`relay.ErrAccountRepoNotFound` -- seems the intent is to provide a
happy-path response for a common error case.
unfortunately the Rev assignment tried to dereference that from `repo`
unconditionally, so it panicked in the allowed-error path.
(still need to test and will verify on my relays)
this change just makes sure `repo` is not nil before we try to pull a `Rev` out of it.
assigning the Rev from the repo follows what's *almost* a boilerplate golang error handler, but is sneakily not handling relay.ErrAccountRepoNotFound -- seems the intent is to provide a happy-path response for a common error case.
unfortunately the Rev assignment tried to dereference that unconditionally, so it panicked in the allowed-error path.
Addresses #1128 and also adds some "hints" to error messages regarding
when to use `goat plc` vs `goat account plc`. There's perhaps still room
for improvement on the command help texts though.
Note: I haven't fully tested a PLC update via `goat account plc submit`
with these changes, yet
Design goals:
- specific to atproto OAuth (not a general-purpose OAuth framework)
- implementation is correct and interoperable with atproto specification
- reasonably complete and flexible, though may make some opinionated
implementation decisions to avoid footguns
- compatible with client SDK (aka, implement `AuthMethod`)
- oriented towards server-side (eg, BFF and integrations)
- supports "just authn" use-cases
Progress:
- [x] basic confidential client demo web interface
- [x] public client mode
- [x] localhost dev client mode
- [x] refactor core types and method attachments (eg, a session-agnositc
OAuthClient struct with http.Client)
- [x] persist token callback (wired to ClientApp)
- [x] make PAR DPoP retries more specific (parse error response)
- [x] resolve XXX and TODO
- [x] multiple session support (and document this pattern)
- [x] document authn-only usecase
- [x] update `randomNonce`
- [x] add doc comments to major functions/types
- [x] more consistent DPoP capitalization in variables (?)
- [ ] fix DID/handle display in demo app
- [ ] ability to embed JWKs in client metadata directly (blocked on
needing `key_ops`?)
- [ ] proactive detection and update (persist) when DPoP nonce changes
- [ ] remember token deletion endpoint as part of session; and add
logout helper which calls it (if defined)
- [ ] mock tests (like service auth has)
following Google style guide: https://google.github.io/styleguide/go/decisions#initialisms
by [the rkey
spec](https://atproto.com/specs/record-key#record-key-syntax):
> restricted to a subset of ASCII characters — the allowed characters
are alphanumeric (A-Za-z0-9), period, dash, underscore, colon, or tilde
(.-_:~)
tilde was missing from this check, which leads to at least [jetstream
becoming unhappy](https://github.com/bluesky-social/jetstream/issues/54)
over valid-but-uncommon rkeys.
this change aligns the check with the spec.
The root cause was that this package was assuming that query and
procedure "params" were required, when they are actually optional.
This corrects them to be optional (pointer type), and skips validation
when nil.
Also adds "minimal" example schemas as regression tests.
Fixes: https://github.com/bluesky-social/indigo/issues/1153
I should probably add parsing helpers to the atproto/syntax package for
MIME type glob patterns, and for "service refs".
This isn't super rigorous yet, and it doesn't support pass-through of
extra fields on `permission` types. But it would catch early/initial
syntax issues, and could be used with `goat` for publishing and
resolving lexicons (eg, basic devex for permission-sets feature).
Accounts sometimes don't have an entry in their account_repo extension
table. This change handles that case for listRepos by including them in
the response but with empty strings for the "Rev" and "Head" fields.
...which probably isn't ultimately correct, but is at least more usable
for clients than missing an entire page of repos because one of them was
affected by this.
placeholder improvement directly related to
https://github.com/bluesky-social/indigo/issues/1143
basically the same as #1142 but that PR only addressed getRepoStatus
---
for what it's worth, the sql join `TODO` suggested in the code here
would have hidden (fixed?) this problem by omitting the affected
accounts, assuming inner-join
Accounts sometimes don't have an entry in their account_repo extension table. This change handles that case for listRepos by including them in the response but with empty strings for the "Rev" and "Head" fields.
...which probably isn't ultimately correct, but is at least more usable for clients than missing an entire page of repos because one of them was affected by this.
placeholder improvement directly related to https://github.com/bluesky-social/indigo/issues/1143
(also for what it's worth, the sql join `TODO` would have hidden (fixed?) this problem by omitting the affected accounts (assuming an inner-join))
this change just makes sure `repo` is not nil before we try to pull a
`Rev` out of it.
assigning the Rev from the repo follows what's *almost* a boilerplate
golang error handler, but is sneakily not handling
`relay.ErrAccountRepoNotFound` -- seems the intent is to provide a
happy-path response for a common error case.
unfortunately the Rev assignment tried to dereference that from `repo`
unconditionally, so it panicked in the allowed-error path.
(still need to test and will verify on my relays)
this change just makes sure `repo` is not nil before we try to pull a `Rev` out of it.
assigning the Rev from the repo follows what's *almost* a boilerplate golang error handler, but is sneakily not handling relay.ErrAccountRepoNotFound -- seems the intent is to provide a happy-path response for a common error case.
unfortunately the Rev assignment tried to dereference that unconditionally, so it panicked in the allowed-error path.
Design goals:
- specific to atproto OAuth (not a general-purpose OAuth framework)
- implementation is correct and interoperable with atproto specification
- reasonably complete and flexible, though may make some opinionated
implementation decisions to avoid footguns
- compatible with client SDK (aka, implement `AuthMethod`)
- oriented towards server-side (eg, BFF and integrations)
- supports "just authn" use-cases
Progress:
- [x] basic confidential client demo web interface
- [x] public client mode
- [x] localhost dev client mode
- [x] refactor core types and method attachments (eg, a session-agnositc
OAuthClient struct with http.Client)
- [x] persist token callback (wired to ClientApp)
- [x] make PAR DPoP retries more specific (parse error response)
- [x] resolve XXX and TODO
- [x] multiple session support (and document this pattern)
- [x] document authn-only usecase
- [x] update `randomNonce`
- [x] add doc comments to major functions/types
- [x] more consistent DPoP capitalization in variables (?)
- [ ] fix DID/handle display in demo app
- [ ] ability to embed JWKs in client metadata directly (blocked on
needing `key_ops`?)
- [ ] proactive detection and update (persist) when DPoP nonce changes
- [ ] remember token deletion endpoint as part of session; and add
logout helper which calls it (if defined)
- [ ] mock tests (like service auth has)
by [the rkey
spec](https://atproto.com/specs/record-key#record-key-syntax):
> restricted to a subset of ASCII characters — the allowed characters
are alphanumeric (A-Za-z0-9), period, dash, underscore, colon, or tilde
(.-_:~)
tilde was missing from this check, which leads to at least [jetstream
becoming unhappy](https://github.com/bluesky-social/jetstream/issues/54)
over valid-but-uncommon rkeys.
this change aligns the check with the spec.