Add Option for Delegation to Any User #134
Conversation
| @@ -50,7 +50,7 @@ func TestCreate(t *testing.T) { | |||
|
|
|||
| func TestSummary(t *testing.T) { | |||
| createJson := []byte("{\"Name\":\"Alice\",\"Password\":\"Hello\"}") | |||
| delegateJson := []byte("{\"Name\":\"Bob\",\"Password\":\"Rob\",\"Time\":\"2h\",\"Uses\":1}") | |||
| delegateJson := []byte("{\"Name\":\"Bob\",\"Password\":\"Rob\",\"Time\":\"2h\",\"Uses\":1,\"AnyUser\":true}") | |||
There was a problem hiding this comment.
It would be useful to test a lot of these with AnyUser: false to ensure that it fails in that case.
There was a problem hiding this comment.
Good point. I added AnyUser: true to preserve the original behavior of the tests, but I agree that we should test the alternate path as well.
|
@kisom please see my test update - I think this logic is best served in its own test, where it won't interfere with the intended behaviors of other tests. |
Current coverage is 55.03%@@ master #134 diff @@
==========================================
Files 12 12
Lines 1766 1779 +13
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 966 979 +13
Misses 615 615
Partials 185 185
|
|
@APTy I won't have time this week to review this, but I will be able to on Monday. |
|
I'm aware of one project abusing the current behavior (no users specified => all users have access). I'll update once they've fixed. Meanwhile, please don't merge this. |
core: Add AnyUser field to DelegateRequest and pass to cache calls keycache: Add AnyUser parameter to AddKeyFromRecord function signature keycache_test: Add tests for AnyUser and update AddKeyFromRecord calls cryptor: Update tests to AddKeyFromRecord to reflect API update cmd/ro: Add bool flag for anyUser parameter Note: This commit was further amended to change the AddKeyFromRecord API to accept keycache.Usage as a parameter, which will make updating usage simpler in the future.
keycache: Remove deprecated behavior in Usage.matches() core: Add check during delegation for a User list or the AnyUser flag tests: Add parameters to conform to new delegation requirements
core: Add unit tests scenarios core: Check for a non-nil, but empty, Users list keycache: Add unit tests
Output before change: cryptor/cryptor_test.go:112: github.com/cloudflare/redoctober/keycache.Usage composite literal uses unkeyed fields
|
This has merge conflicts and needs to be rebased off master. |
This change resolves #119 by adding an explicit option (
"AnyUser") for a delegator to allow any user to decrypt requested data. This improves on the current design, where a delegator must specify an empty list of users for the same effect.Adding the AnyUser Option
Commit 4093134 adds the initial enhancement by extending the API for
Cache.AddKeyFromRecord(). At this point, the function signature for this function is getting pretty heavy, but as it's only used once incore/core.go, this is probably OK (The test dependencies may be another matter). We document that delegating to an empty list of users is deprecated functionality, but still support it. Also adds CLI support for the same flag.Edit: The function signature for
Cache.AddKeyFromRecord()now accepts aUsageobject to simplify it's API.Enforcing the AnyUser Option
Commit 5549d4c adds enforcement (thereby breaking backwards-compatibility) of the flag by returning an error in
core/core.goif we don't receive a list ofUsersor if theAnyUserflag is not set. This break the old, undesired behavior, and have to update quite a few test vectors to reflect the new API.Thoughts on rolling these changes out incrementally to allow applications time to upgrade to the modified
/delegateAPI?