-
Notifications
You must be signed in to change notification settings - Fork 35
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
WIP: Jw3t auth #1538
base: v1.4.0
Are you sure you want to change the base?
WIP: Jw3t auth #1538
Conversation
|
||
type JW3THeader struct { | ||
Algorithm string `json:"algorithm"` | ||
AddressType string `json:"address-type"` |
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 - for the other types that we expose, we use underscores instead of dashes. Maybe we can do that here to keep things consistent?
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 would agree, I am just following the community proposed standard https://hackmd.io/Xp1-M5WVRFybdLwjWqXdEw?view#Web3-Authentication-Design-Draft and that is using -
instead.
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.
indeed it was meant to be underscore. The TOML format was set to underscore but it was not updated for the JSON format. It is fixed in the doc.
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.
@hamidra thanks for the input we will update to _
then
http/auth/service.go
Outdated
func decodeJW3T(jw3t string) (*JW3THeader, *JW3TPayload, []byte, error) { | ||
fragments := strings.Split(jw3t, ".") | ||
if len(fragments) != 3 { | ||
return nil, nil, nil, errors.New("bad JW3T format") |
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 suggest we use constant errors for cases like these.
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.
yep, sounds good
http/auth/service.go
Outdated
ProxyType string | ||
} | ||
|
||
type Auth struct { |
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.
Can we have an interface for this so we can use mocks in the tests?
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.
Indeed, will do that
http/auth/service.go
Outdated
return nil, err | ||
} | ||
|
||
key, err := types.CreateStorageKey(meta, identity.ProxyPallet, identity.ProxiesMethod, proxyPublicKey) |
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 can move all this proxy stuff in a separate service that deals with this. I created one on my identities branch, for now I only implemented this:
type API interface {
GetProxies(ctx context.Context, accountID *types.AccountID) (*types.ProxyStorageEntry, error)
}
Lemme know how you want to proceed.
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 certain we will do that, since you were working on the proxies interaction, I didnt want to step on each other toes. I was going to do add that as part of your interface once it was in.
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.
@cdamian thanks for the review, If you have any suggestions on the admin/nonadmin or any other code structure approach overall would be great to hear.
Let sync on how to best integrate your work items and this one as seamlessly as possible.
|
||
type JW3THeader struct { | ||
Algorithm string `json:"algorithm"` | ||
AddressType string `json:"address-type"` |
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 would agree, I am just following the community proposed standard https://hackmd.io/Xp1-M5WVRFybdLwjWqXdEw?view#Web3-Authentication-Design-Draft and that is using -
instead.
http/auth/service.go
Outdated
ProxyType string | ||
} | ||
|
||
type Auth struct { |
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.
Indeed, will do that
http/auth/service.go
Outdated
func decodeJW3T(jw3t string) (*JW3THeader, *JW3TPayload, []byte, error) { | ||
fragments := strings.Split(jw3t, ".") | ||
if len(fragments) != 3 { | ||
return nil, nil, nil, errors.New("bad JW3T format") |
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.
yep, sounds good
http/auth/service.go
Outdated
return nil, err | ||
} | ||
|
||
key, err := types.CreateStorageKey(meta, identity.ProxyPallet, identity.ProxiesMethod, proxyPublicKey) |
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 certain we will do that, since you were working on the proxies interaction, I didnt want to step on each other toes. I was going to do add that as part of your interface once it was in.
No description provided.