Replace custom random number generator with UUID

[solid-akb]

This PR replaces our custom random number generator with uuid.

Note: This PR introduces a new package uuid to the codebase, which we have audited and is already used by our other SDKs

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
solid-client-js	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 1, 2023 at 6:18PM (UTC)

[ThisIsMissEm]

Looks good, the only change to make is the comment about return uuidv4().toString().substring("0.".length);

Though I would like to maybe wait a moment until Nic A.S. or @matthieubosquet can respond about the policy / matcher IRIs and why the format they are in is at all significant. I think ESS is actually even hashing these IRIs (I wasn't able to find anything in my pod that had an ACR that contained the strings we use in this code)

[ThisIsMissEm]

Suggested change

	return uuidv4().toString().substring("0.".length);
	return uuidv4();

The toString / substring thing was due to the behaviour of Math.random(), see the screenshot above.

[solid-akb]

Ahh roger that!

[ThisIsMissEm]

I think all these identifiers for newIriSuffix can be removed, and we can just always generate a uuidv4() in each of createResourceMatcherFor and createResourcePolicyFor probably should just be the matcher IRI with the hash replaced with a uuid, rather than appending to the IRI.

e.g., the current implementation may be leaking information about the actor & the relationship in the policy or matcher IRI, when that value carries no significance, and it's really just a "makes the policy nice to read/look at"

Though that may be a wider refactor, so deferring to @NSeydoux

[solid-akb]

So if I understand this comment better, we can replace lines 867-870 with

const newIriSuffix = uuidv4();

Is that correct?

Will wait for @NSeydoux response before pushing updates.

Beyond just changing the value to the code mentioned above, would we need to change anything else? Specifically, do we have code in other places that relies on having the "copy_without", "actorRelationToExclude" or "actorTo Exclude" in the IriSuffix?

[ThisIsMissEm]

Beyond just changing the value to the code mentioned above, would we need to change anything else? Specifically, do we have code in other places that relies on having the "copy_without", "actorRelationToExclude" or "actorTo Exclude" in the IriSuffix?

as far as I can tell, no, and in fact, ESS will make those IRIs for subject/object opaque, so they won't be returned back as-is anyway

[NSeydoux]

There shouldn't be any hard-coded dependencies on the resource names, or reliance on any in-IRI semantics in the low-level API (we know there is some in the high-level API, but that's irrelevant here). I don't think ESS changes these IRIs, for instance when using the permissions app you can give a human-friendly name to a policy and it is persisted as-is. Finally, I think there isn't any information leak here: any user authorized to see the ACR in the first place would be able to see any actor mentioned in it, so it being embedded in an IRI doesn't add any information to what the resource already contains. It is really just for debug purpose.

All that said, it is a good practice to not have semantics in IRIs, so provided there isn't any reliance on this piece of logic, I think we can go forward with Emelia's suggestion and just use the uuid.

[ThisIsMissEm]

I've also subsequently removed the rather brittle test for encoding hashes in the ACP IRIs, as we don't do that any more, and the only place this may come in is through existing IRIs maybe.

[solid-akb]

This looks good now with the irrelevant tests removed. 👍