feat: Packager now holds multiple Packers for runtime format selection #717
Conversation
Fixes hyperledger-archives#160 Signed-off-by: Filip Burlacu <[email protected]>
|
Note: need to reconcile urlencoding vs rawurlencoding, or allow Edit: done |
pkg/didcomm/envelope/api.go
Outdated
| @@ -26,7 +52,7 @@ type Packager interface { | |||
| // []byte: The packed message | |||
| // | |||
| // error: error | |||
| PackMessage(envelope *Envelope) ([]byte, error) | |||
| PackMessage(envelope *packer.Envelope) ([]byte, error) | |||
There was a problem hiding this comment.
There is a package called envelope but this signature has an Envelope defined in packer?
What is the intended purpose of the Envelope package?
There was a problem hiding this comment.
I could swap the package names - I needed to pull Envelope out of envelope to avoid an import cycle.
There was a problem hiding this comment.
I renamed envelope (containing just the packager) to packager and packer (previously crypto) to envelope
Moopli
force-pushed
the
160
branch
2 times, most recently
from
fbe8ee2 to
ad2316c
Compare
Codecov Report
@@ Coverage Diff @@
## master #717 +/- ##
==========================================
+ Coverage 90.6% 90.99% +0.39%
==========================================
Files 82 82
Lines 4777 4840 +63
==========================================
+ Hits 4328 4404 +76
+ Misses 252 240 -12
+ Partials 197 196 -1
Continue to review full report at Codecov.
|
| @@ -79,7 +111,7 @@ func TestBaseKMSInPackager_UnpackMessage(t *testing.T) { | |||
| // It should fail since Recipient keys are not found in the KMS | |||
| _, err = packager.UnpackMessage(packMsg) | |||
| require.Error(t, err) | |||
| require.EqualError(t, err, "failed from decrypt: failed to decrypt message: key not found") | |||
There was a problem hiding this comment.
why remove the specific error check?
There was a problem hiding this comment.
Now that the message is partially decoded before the Packer unpacks it, the failure case is different, and occurs in getEncodingType()
| require.NoError(t, err) | ||
| _, err = packager.UnpackMessage(nil) | ||
| require.Error(t, err) | ||
| require.EqualError(t, err, "failed from decrypt: failed to decrypt message: unexpected end of JSON input") |
There was a problem hiding this comment.
why remove expected detailed error.. this error message should be specific.
There was a problem hiding this comment.
This is another case where the error now happens inside getEncodingType()
| // Package envelope manages the handling of DIDComm raw messages in JWE compliant envelopes. | ||
| // The aim of this package is build these envelopes and parse them. They are mainly used as | ||
| // wire-level wrappers of 'payloads' used in DID Exchange flows. | ||
|
|
||
| // PackagerCreator method to create new outbound dispatcher service | ||
| type PackagerCreator func(prov Provider) (Packager, error) | ||
| type PackagerCreator func(prov PackerProvider) (Packager, error) | ||
|
|
||
| // Packager provide methods to pack and unpack msg | ||
| type Packager interface { |
There was a problem hiding this comment.
now that Crypter is renamed to Packer, this Packager interface becomes an extra layer that we can skim.
There was a problem hiding this comment.
It now has the purpose of selecting which pack/unpack algorithm to use, allowing the agent to support communicating using multiple message formats for compatibility - so it's not a layer we can pull out.
pkg/didcomm/envelope/api.go
Outdated
| @@ -6,12 +6,16 @@ SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| package envelope | |||
There was a problem hiding this comment.
Is it correct to observe that this package is only needed by the framework (supplying the injectable API)?
There was a problem hiding this comment.
In talking with @Baha-sk we've decided to go the other way - Packager does not need to be injectable because pack/unpack format configuration is done through WithDefaultPacker() and WithPackers(). Where do you think Packager should move? Without Packager injection we only need the interface for mocking.
There was a problem hiding this comment.
Packager will hold the list of Packers including a default one, it will also decide which one to use based on the envelope type. Users don't need to inject it as it's a utility interface that's required by the framework.
Users can inject Packers and a DefaultPacker to support their own implementation.
Moopli
force-pushed
the
160
branch
6 times, most recently
from
a1fa70c to
c29c5d1
Compare
| @@ -66,7 +66,7 @@ func TestNilEncryptSenderJwk(t *testing.T) { | |||
| require.Empty(t, l) | |||
| require.Empty(t, m) | |||
|
|
|||
| pld, err := crypter.Encrypt([]byte(""), someKey[:], [][]byte{someKey[:]}) | |||
| pld, err := crypter.Pack([]byte(""), someKey[:], [][]byte{someKey[:]}) | |||
There was a problem hiding this comment.
why is the variable name still crypter ?
There was a problem hiding this comment.
I'll go over all other uses of 'crypter' too.
There was a problem hiding this comment.
this should be done now.
pkg/didcomm/packager/api.go
Outdated
| "github.com/hyperledger/aries-framework-go/pkg/didcomm/envelope" | ||
| ) | ||
|
|
||
| // Package packager manages the handling of DIDComm raw messages in JWE compliant envelopes. |
There was a problem hiding this comment.
I think the top level Packager interface is agnostic of JWE?
There was a problem hiding this comment.
what do you think I should say instead for "DID Exchange flows"? Since this format is used for all protocol communications.
There was a problem hiding this comment.
I don't see "DID Exchange" - I saw "DIDComm".
pkg/didcomm/packager/api.go
Outdated
| package packager | ||
|
|
||
| import ( | ||
| "github.com/hyperledger/aries-framework-go/pkg/didcomm/envelope" |
There was a problem hiding this comment.
Is there a reason for a separate envelope package? Does it now only have a struct?
There was a problem hiding this comment.
Envelope struct, Packer interface, and Packer Creator.
There was a problem hiding this comment.
It's separate mainly to avoid an import cycle - which iirc was a cycle only because of tests, so I'll see if I can fix that.
There was a problem hiding this comment.
I think I should keep the packages separate, but I can rename envelope to packer - I'd named it packer earlier. I'll work out how to put the Envelope struct inside pkg/didcomm/packager without an import cycle, which I believe only came about due to tests.
There was a problem hiding this comment.
I've resolved this by pulling Envelope out into pkg/didcomm/common/transport
pkg/didcomm/packager/packager.go
Outdated
| } | ||
|
|
||
| // BasePackager is the basic implementation of Packager | ||
| type BasePackager struct { |
There was a problem hiding this comment.
Is there a need for the prefix Base ?
There was a problem hiding this comment.
No, since it's not injectable we don't expect others.
Will fix.
There was a problem hiding this comment.
Then again there is the distinction between this and the interface (the interface is Packager), since we also have a mock packager. The interface is what everything consumes.
I could make the struct definition private.
There was a problem hiding this comment.
The interface and struct shouldn't be in the same package.
The struct needs to be exported since it is returned by New (hiding these structs is annoying to the developer).
There was a problem hiding this comment.
I can return the interface instead of a pointer to the struct
There was a problem hiding this comment.
Where should I move the interface? It's consumed all over the codebase, particularly in tests which usually use the mock packager.
There was a problem hiding this comment.
I've resolved this by pulling the Packager interface out into pkg/didcomm/common/transport
pkg/framework/aries/framework.go
Outdated
| @@ -208,7 +211,8 @@ func (a *Aries) Context() (*context.Provider, error) { | |||
| context.WithInboundTransportEndpoint(a.inboundTransport.Endpoint()), | |||
| context.WithStorageProvider(a.storeProvider), | |||
| context.WithTransientStorageProvider(a.transientStoreProvider), | |||
| context.WithCrypter(a.crypter), | |||
| context.WithDefaultPacker(a.packer), | |||
There was a problem hiding this comment.
Should I assume that a.packer must also be in the a.packers list in L215?
There was a problem hiding this comment.
Packager handles that - if the default packer is not in the list, it's added.
There was a problem hiding this comment.
Specifically - each Packer defines an encoding type string (returned by Packer.EncodingType()) which is used to key into the packer map. So if a.packer's type is the type of one of the elements of a.packers, then it's assumed to be in the list already.
| @@ -92,11 +90,20 @@ func TestFramework(t *testing.T) { | |||
| WithKMS(func(ctx api.Provider) (api.CloseableKMS, error) { | |||
| return &mockkms.CloseableKMS{SignMessageValue: []byte("mockValue")}, nil | |||
| }), | |||
| WithCrypter(func(ctx crypto.Provider) (crypto.Crypter, error) { | |||
| return &didcomm.MockAuthCrypt{EncryptValue: nil}, nil | |||
| WithDefaultPacker(func(ctx envelope.Provider) (envelope.Packer, error) { | |||
There was a problem hiding this comment.
Is this really "Default" or is it the outbound packer?
I mean the framework puts defaults in for the Packers list too :).
There was a problem hiding this comment.
Good point, I'll rename.
There was a problem hiding this comment.
If the outbound packer isn't specified, which packer from s.packers is used?
There was a problem hiding this comment.
Though it is also a default in the sense that it's included in the list of inbound packers, since it makes little sense to send messages using a format you're unwilling to read.
There was a problem hiding this comment.
If the outbound packer isn't specified, then the framework falls back to initializing the default, which isn't inside s.packers. packager.New requires a default packager to be provided. I'll add an error check in the constructor if the value is nil.
There was a problem hiding this comment.
instead of "Default" or "Outbound" it is now just "Packer", since it's used for both in/outbound.
The packer list is now InboundPackers, since they're used inbound only right now. #725 is for making those packers usable outbound as well, by remembering the format to use for a particular connection.
Packager can select Packer for decryption based on the Typ field of the input message. Fixes hyperledger-archives#273 Signed-off-by: Filip Burlacu <[email protected]>
| // Packager is the basic implementation of Packager | ||
| type Packager struct { | ||
| packer packer.Packer | ||
| inboundPackers map[string]packer.Packer |
There was a problem hiding this comment.
why inboundPackers? it's just a list of Packers
a Packer does pack/unpack, regardless of the direction of the message.
any reason for the direction word in the variable inoundPackers?
There was a problem hiding this comment.
The packer seems to be the only argument doing both pack and unpack.
The inbound packers seems to be unpack only. Perhaps a better name would be unpackers :).
There was a problem hiding this comment.
the map of packers should not be meant for inbound only, during a connection, if Agent A talks with B in JWE compliant format and Agent C in legacy packer format, then Agent A should use the right outbound Packer for each connection (not just for replying to incoming messages). The the map of Packers is not meant to be for inbound only (Unpack()) calls.
also, packer field above was agreed to be the DefaultPacker which is meant to be used for new connections. For pre-existing/saved connections, the agent should store the corresponding Packer to be used for all subsequent calls (outbound and inbound).
so the word inbound in inboundPacker does not correctly represent the map of Packers as they should not be used for inbound calls only.
There was a problem hiding this comment.
Default doesn’t seem like a good prefix -if we use that then there is a default default packer set by the framework package if the option is not provided.
| require.NoError(t, err) | ||
|
|
||
| mockedProviders := &mockProvider{ | ||
| storage: nil, |
There was a problem hiding this comment.
no need to set nil variables
| storage *mockstorage.MockStoreProvider | ||
| kms kms.KeyManager | ||
| packers []packer.Packer | ||
| outboundPacker packer.Packer |
There was a problem hiding this comment.
not sure outboundPacker has a meaning, it's just a Packer that should be able to do Pack/Unpack operations.
why add the direction in the variable name?
There was a problem hiding this comment.
I think we should combine these options into one to reduce confusion (and API surface).
#764.
Renames Crypters to Packers, and gives the Packager a list of Packers, for all formats the agent supports (elements are configurable and injectable by client), and a reference to the default Packer (also configurable and injectable) to use for encryption.
By default, the JWE and Legacy Packers are both enabled, available for decrypting messages.
Fixes #160
Fixes #273