Skip to content
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

Proposal for breaking change in jsonld document loader #50

Closed
mattcollier opened this issue Oct 11, 2019 · 6 comments
Closed

Proposal for breaking change in jsonld document loader #50

mattcollier opened this issue Oct 11, 2019 · 6 comments

Comments

@mattcollier
Copy link
Contributor

It is currently considered best practice not to have jsonld document loaders fetch things from the Internet. The default document loader in Bedrock is designed to fetch things from the Internet.

https://github.com/digitalbazaar/bedrock/blob/master/lib/jsonld.js#L20
https://github.com/digitalbazaar/jsonld.js/blob/master/lib/documentLoaders/node.js

It has also been discussed that jsonld should not be baked into the base Bedrock module and should be in a module like bedrock-jsonld.

In order to make Bedrock secure by default, I propose a breaking change that implements a secure document loader.

We could at the same remove bedrock.jsonld all together and require that a new bedrock-jsonld module be installed and used instead.

How should we approach this issue?

@dlongley
Copy link
Member

@mattcollier,

If we're going to do a breaking change on bedrock, yes, we should absolutely split jsonld out so we can have that evolve independently. I'm +1 for breaking it out and making bedrock-jsonld have a secure document loader by default. And, really, bedrock-jsonld should just be bedrock-jsonld-documentloader (or something better named). We just want to provide a document loader that hooks into the bedrock shared internals -- we don't need to wrap the whole library.

@dlongley
Copy link
Member

Also, with that approach, we can build a fancy doc loader that is secure by default but easily configured with an API for adding static contexts/docs to be loaded or dynamic loaders to pull things from the network (or wherever). IOW, we could implement this in a BSD lib and then implement a bedrock wrapper that hooks it into bedrock internals/shares it across modules. Of course, this increases scope. If we're able to create something that allows us to put this approach on the roadmap and just take a step towards it (whilst getting better security) we should do that instead.

@DB-RobertB
Copy link

Anything that allows us a more flexible modular structure for the wallet, issuer, and verifier is good.
When looking at very lightweight mobile or web solutions, it's useful to have smaller build sets with less breaking dependencies. I don't know enough about our code base, but I do think that if we can keep as much of our detailed knowledge in our cloud web services, and make the presentation parts just trivial mobile code that any contract engineer can build, just like we do with web sites, then we lower the learning threshold for bringing additional (contract) engineers into the fold.

@mattcollier
Copy link
Contributor Author

@dlongley @davidlehn @gannan08 I'm trying to think about what the first, easiest step here. We have lots of existing code that just uses bedrock.jsonld and the document loader cache along with the bedrock-foo-context modules we have: https://github.com/digitalbazaar?utf8=%E2%9C%93&q=bedrock+context&type=&language=

The context modules add documents to a CONTEXT object as well as adding a constant that maps to the context URL.

Seems like we would want a bedrock.jsonld.CONTEXT and bedrock.jsonld.urls(?) for this purpose? It follows that this config would be setup by bedrock-jsonld.

Is the aim that we should have a jsonld dependency in all the modules that use jsonld or that it would be a subdependency of some bedrock-foo module?

OR...

Is it (as was suggested) bedrock-jsonld-document-loader which has a peerDependency for jsonld, provides places to put context document and urls and only provides a documentLoader and does not surface jsonld APIs in any way.

@mattcollier
Copy link
Contributor Author

Addressed in: #52

@mattcollier
Copy link
Contributor Author

This has landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants