-
Notifications
You must be signed in to change notification settings - Fork 191
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
Echo register auth #740
Echo register auth #740
Conversation
7676520
to
b8c32fa
Compare
|
||
actor EchoRegisterService { | ||
private let httpClient: HTTPClient | ||
private let projectId: String | ||
private let clientId: String | ||
private let logger: ConsoleLogging | ||
private let environment: APNSEnvironment | ||
private let echoAuthenticator: EchoAuthenticating | ||
// DID method specific identifier | ||
private var clientIdMutlibase: String { | ||
return clientId.replacingOccurrences(of: "did:key:", with: "") |
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.
may be use DIDKey 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.
in what way?
actually clientIdStorage could return DIDKey?
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.
i mean:
let clientId: DIDKey
and after you can get the string with did(prefix: Bool, variant: DIDKeyVariant)
I created DIDKey type to avoid of using string keys. Actually good idea to return DIDKey from clientIdStorage
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.
clientID cannot be DIDkey because we could confuse the variant
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, few comments
@@ -12,6 +12,7 @@ public protocol HTTPService { | |||
var method: HTTPMethod { get } | |||
var body: Data? { get } | |||
var queryParameters: [String: String]? { get } | |||
var headerFields: [String: String]? { get } |
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.
may be additionalHeaders
, because it's not represent all request headers as we are setting Content-Type
separately after
@@ -122,7 +122,7 @@ private extension InvitationHandlingService { | |||
let inviterAccount = try await identityClient.resolveIdentity(iss: claims.iss) | |||
// TODO: Should we cache it? | |||
let inviteePublicKey = try await identityClient.resolveInvite(account: inviterAccount) | |||
let inviterPublicKey = invite.inviterPublicKey.did(prefix: false, variant: .X25519) | |||
let inviterPublicKey = invite.inviterPublicKey.multibase(variant: .ED25519) |
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.
why you changed for ED25519
? inviterPublic key is creating by createX25519KeyPair
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.
also could you change it please to .did
with prefix
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.
was a typo, added did prefix too
Description
Authorization
headerResolves # (issue)
How Has This Been Tested?
Due Dilligence