-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: Implement SourceHub ACP #2657
Changes from all commits
03ff346
1c95871
8a21968
77f8b1b
5ec5678
f55340f
bb229e9
318ec88
6a220a9
4eb9fe3
07d116a
f7f5ff0
116347e
d0ef651
3d084b2
c395e8d
0c548a1
86b6eb7
99c7d79
f0f3c88
4ccfeaa
59901aa
da3c973
f5c2726
7b355f8
97bd37a
994aec1
fdd5a25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -436,8 +436,14 @@ To perform authenticated operations you will need to build and sign a JWT token | |
|
||
- `sub` public key of the identity | ||
- `aud` host name of the defradb api | ||
|
||
> The `exp` and `nbf` fields should also be set to short-lived durations. | ||
- The `exp` and `nbf` fields should also be set to short-lived durations. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Perhaps a better separation of local and sourcehub acp specific options as they might use them interchangeably or be confusing if an option is needed for xyz acp type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no local only fields. And the sourcehub only ones are very clearly documented as such There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a sourcehub acp dedicated section somewhere in this markdown file guiding users how to setup sourcehub acp and all the bits they would need, and how to get them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think that should be covered by SourceHub documentation. We have to bear in mind that SourceHub is a separate service/process too, and it's version is not tied to Defra releases. By duplicating documentation in the Defra repo it will likely be incorrect at various times as SourceHub updates. |
||
|
||
Additionally, if using SourceHub ACP, the following must be set: | ||
- `iss` should be set to the user's DID, e.g. `"did:key:z6MkkHsQbp3tXECqmUJoCJwyuxSKn1BDF1RHzwDGg9tHbXKw"` | ||
- `iat` should be set to the current unix timestamp | ||
- `authorized_account` should be set to the SourceHub address of the account signing SourceHub transactions on your | ||
behalf - WARNING - this will currently enable this account to make any SourceHub as your user for the lifetime of the | ||
token, so please only set this if you fully trust the node/account. | ||
|
||
The JWT must be signed with the `secp256k1` private key of the identity you wish to perform actions as. | ||
|
||
|
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.
suggestion: I think at this point we might as well just make another
job
in this same workflow file calledrun-sourcehub-tests
with it's own strategy only running the three clients (client-type: [go, http, cli]
). Will the extraif: ${{ matrix.acp-type == 'source-hub' }}
conditions from this strategy. Then under theneeds: ...
of theupload-coverage
job you can add another namerun-sourcehub-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.
I do prefer the current setup. Everything works in the same way and is declared in the same location. It is getting a bit lengthy, but is better than the alternative atm.