Generalize userid authentication #2589
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
network
harbu
force-pushed
the
generalize-userid-authentication
branch
from
3a65491
to
07d59dc
Compare
harbu
force-pushed
the
generalize-userid-authentication
branch
from
07d59dc
to
3500245
Compare
harbu
force-pushed
the
generalize-userid-authentication
branch
2 times, most recently
from
64fdf85
to
d3c7960
Compare
harbu
force-pushed
the
generalize-userid-authentication
branch
3 times, most recently
from
a7b21f1
to
3050b54
Compare
harbu
commented
Sep 19, 2024
| @@ -356,9 +358,9 @@ export class StreamRegistry { | |||
| if (isPublicPermissionQuery(query)) { | |||
| return this.streamRegistryContractReadonly.hasPublicPermission(streamId, permissionType) | |||
| } else if (query.allowPublic) { | |||
| return this.streamRegistryContractReadonly.hasPermission(streamId, toEthereumAddress(query.user), permissionType) | |||
| return this.streamRegistryContractReadonly.hasPermission(streamId, binaryToHex(query.user), permissionType) | |||
There was a problem hiding this comment.
is binaryToHex needed?
harbu
commented
Sep 19, 2024
| } else { | ||
| return this.streamRegistryContractReadonly.hasDirectPermission(streamId, toEthereumAddress(query.user), permissionType) | ||
| return this.streamRegistryContractReadonly.hasDirectPermission(streamId, binaryToHex(query.user), permissionType) |
There was a problem hiding this comment.
is binaryToHex needed?
harbu
commented
Sep 19, 2024
| try { | ||
| return await this.streamRegistryContractReadonly.hasPermission(streamId, userAddress, streamPermissionToSolidityType(StreamPermission.PUBLISH)) | ||
| const permission = await this.streamRegistryContractReadonly.getPermissionsForUserId(streamId, userId) |
There was a problem hiding this comment.
Consider renaming to
permissionsRecord
harbu
commented
Sep 19, 2024
| try { | ||
| return await this.streamRegistryContractReadonly.hasPermission(streamId, userAddress, streamPermissionToSolidityType(StreamPermission.SUBSCRIBE)) | ||
| const permission = await this.streamRegistryContractReadonly.getPermissionsForUserId(streamId, userId) |
There was a problem hiding this comment.
Consider renaming to
permissionsRecord
harbu
commented
Sep 19, 2024
| @@ -42,7 +42,7 @@ export class PublisherKeyExchange { | |||
| private readonly store: LocalGroupKeyStore | |||
| private readonly authentication: Authentication | |||
| private readonly logger: Logger | |||
| private readonly erc1271ContractAddresses = new Set<EthereumAddress>() | |||
| private readonly erc1271ContractAddresses = new ObjectSet<Uint8Array>((u) => binaryToHex(u, true)) | |||
There was a problem hiding this comment.
Move back to Set<EthereumAddress>. Follow-up question: do we need ObjectSet at all?
harbu
commented
Sep 19, 2024
packages/sdk/src/permission.ts
Outdated
| @@ -40,7 +39,7 @@ export const PUBLIC_PERMISSION_ADDRESS = '0x000000000000000000000000000000000000 | |||
|
|
|||
| export type PermissionQueryResult = { | |||
| id: string | |||
| userAddress: EthereumAddress | |||
| userAddress: string | |||
There was a problem hiding this comment.
Should this be userId?
harbu
force-pushed
the
generalize-userid-authentication
branch
3 times, most recently
from
373e508
to
6a89198
Compare
…ID from that package
…unicator (#2737) ## Summary Removed unused EventEmitter from the RpcCommunicator
## Background
In `jest` we can't use `toEqual` assertions for binary data. E.g. this
kind of assertion fails:
```
expect(new Uint8Array([1, 2, 3])).toEqual(Buffer.from([1, 2, 3]))
```
Typically test shouldn't care whether the data is stored in `Uint8Array`
or a `Buffer`. Therefore it would be ok to have that kind of assertion
in a test case.
## Changes
Added `toEqualBinary` jest custom matcher to `test-utils` package. This
allows us to add assertions about binary data like this:
```ts
expect(new Uint8Array([1, 2, 3])).toEqualBinary(new Uint8Array([4, 5, 6]))
```
If the assertion fails, it prints a readable error message:
```
Binaries are not equal
Expected: "0x040506"
Received: "0x010203"
```
## How to use
### Jest setup
```js
'@streamr/test-utils/setupCustomMatchers'
```
to `setupFilesAfterEnv` in `jest.config.js`
### Type information
```ts
import '@streamr/test-utils/customMatcherTypes'
```
e.g. to `test/types/global.d.ts`
### Karma setup (if needed)
```js
const { customMatchers } = require('@streamr/test-utils')
expect.extend(customMatchers)
```
to `karma-setup.js`
## Dependency update
- Bump `jest` from v29.5 to v29.7.
## Future improvements
- Maybe the error message could include more info, e.g.:
- lengths of the inputs
- the offset where the contents differ
- UTF8 text representation of the data if it seems to be ASCII data
github-actions
bot
added
test-utils
teogeb
force-pushed
the
generalize-userid-authentication
branch
from
26a4d5b
to
993f38e
Compare
teogeb
force-pushed
the
generalize-userid-authentication
branch
from
bf75cef
to
3010613
Compare
teogeb
force-pushed
the
generalize-userid-authentication
branch
from
7fba5e1
to
bb5da43
Compare
This was referenced Sep 25, 2024
teogeb
added a commit
that referenced
this pull request
Sep 26, 2024
Added new helper type `UserID`, which is currently just an alias to `EthereumAddress`. Replaced the `EthereumAddress` type with this new type in all places where the address is a user id. Also unified some variable/parameter names. This is a preparation PR for #2589, in which the `UserID` type will be changed to `UInt8Array`. ## Other changes Changed argument type of `StreamrClient#addEncryptionKey` from `EthereumAddress` to `string` as the method is part of our public API.
teogeb
added a commit
that referenced
this pull request
Sep 26, 2024
Use random user id in tests. Changed the usages of `randomEthereumAddress` to `randomUserId`, and in some cases changed static user id to randomly generated user id. This is a preparation PR for #2589, in which the `randomUserId()` will return a `UInt8Array`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Please provide a summary of the changes and a motivation if applicable.
Changes
Provide a bullet list of individual changes. Leave this section out if change
set is small and obvious from summary.
Limitations and future improvements
TODO
randomUserIdfunctions to use random length instead of Ethereum-compatible 20 bytestype UserIDtype fromdhttotrackerless-networkaddressanduserAddressand replace with e.g.userIdin project.randomUserId()for testsuserAddressfield deprecated in The Graph? Should we use theuserIdfield, or both?toEthereumAddressutility so that we wouldn't needisEthereumAddressByteArrayfunction?Checklist before requesting a review
TODOcomments left behind are meant to be left in.