-
Notifications
You must be signed in to change notification settings - Fork 635
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
lex: add more identity helpers #2264
Conversation
I was thinking about the OOB did-management use case too - it would be nice if an authenticated refreshIdentity() against a PDS would trigger it to emit |
171567b
to
1121c3a
Compare
12030a3
to
f1d86d1
Compare
5ed66c1
to
8c9a942
Compare
"handle": { | ||
"type": "string", | ||
"format": "handle", | ||
"description": "The validated handle of the account; or 'handle.invalid' if the handle did not bi-directionally match the DID document." |
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.
is it out of character for this to be a magic string? instead leave the whole handle
clause optional and only present if valid?
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.
or make an explicit {"handle.invalid":true}
member to the result object?
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 do the handle.invalid
thing pretty consistently across the stack: it is mentioned in the spec, processed specially in the app, etc.
arguably we might want to signal "this DID does not declare a handle at all" separately from "the handle doesn't validate bi-directionally", for things like service DIDs. but this is the established pattern at this point.
"lexicon": 1, | ||
"id": "com.atproto.identity.defs", | ||
"defs": { | ||
"atprotoIdentity": { |
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.
com.atproto.identity.defs#atprotoIdentity
has a bit of a weird ring to me. like it already has atproto.identity
in the NSID & then just repeats itself. Maybe #identityDetails
or #identityDetailed
or something to make it more descriptive of what's in there.
But also this is only used in refs (not open unions) and lots of our defs
schema fragments end up having weird/long names. So probably fine.
I will say it reminds me a bit of com.atproto.repo.strongRef
in being such a foundational concept. We hoisted that one up to a top level NSID. I kinda regret that because it's the only one of its kind, but it could make sense to do something similar here?
Not really sure, just airing some ideas, this isn't a huge hangup for me.
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.
com.atproto.identity.defs#identity
felt a bit weird to me, but is also an option. Looking at it again, having atproto
as a prefix is kind of weird, given this is in the com.atproto.*
namespace.
I think I lean away from top-level NSIDs like com.atproto.repo.strongRef
for this sort of thing. Especially this one, i'm not sure it will actually get re-used often? Passing around the full DID Document as unknown
isn't super ergonomic.
(for strongRef
, in the mid-term i'm hoping we can extend the at://
syntax to accommodate the strongRef use-case, and not have such a basic building-block require a Lexicon reference)
"format": "handle", | ||
"description": "The validated handle of the account; or 'handle.invalid' if the handle did not bi-directionally match the DID document." | ||
}, | ||
"tombstoned": { |
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.
Since we got rid of the tombstone
event, I'm not sure we have "tombstone" anywhere in the spec (except in PLC).
Tombstone as a concept is weird to apply to some DID methods such as did:web. I wonder if it may make sense to change the name of this property to something more generalized like doesNotResolve
or something 🤔
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.
oh, good call. my memory was that the DID Core specification used the term "tombstone", but it actually uses the term "deactivate". I think it would be confusing for us to use "deactivated" because that is a term we use for atproto account hosting status.
The other approach here is to have it be an distinct error response type, instead of a flag. I'm going to go in that direction.
I renamed Removed the I guess we could have |
A last self-review comment: in theory In favor of a separate |
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.
looks great ✅
The current push/use-case for this is an identity cache microservice.
These are ideas/follow-ups from discussions with @dholms during account migration design.
One example use-case
refreshIdentity
isdid:web
users, or folks that manage theirdid:plc
out-of-band. In these cases, users need a way to tell their PDS that their identity has updated and should be re-resolved server-side. Another is somebody who fixes their handle, but still appears invalid from AppView; that might not result in an#identity
event on firehose, and they might want to poke the AppView to refresh to fix it. In an expansive sense, this could be implemented by every atproto service (Relay, AppView, Mod Service, etc), with a rate-limit. We probably wouldn't want to start with that though; I left it ambiguous with "might require auth, and could be ignored".The second is a mechanism to resolve a DID doc and handle together. This is intended for clients which don't want to bother talking to PLC, and just want to ask their PDS to resolve an identity for them. This could become more important/useful in the future if we add more DID methods. Really it just feels like a missing piece for now, in that we have
resolveHandle
but notresolveDid
(this rolls up the functionality of the later).