fix: Make requests with no identity work with "*" target #3278
Conversation
shahzadlone
added
bug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3278 +/- ##
===========================================
+ Coverage 77.95% 78.07% +0.11%
===========================================
Files 382 382
Lines 35364 35371 +7
===========================================
+ Hits 27568 27614 +46
+ Misses 6148 6122 -26
+ Partials 1648 1635 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
shahzadlone
force-pushed
the
lone/test/add-tests-for-non-identity-actors-after-any-actor-request
branch
4 times, most recently
from
22b2dfa
to
1ceda7a
Compare
shahzadlone
force-pushed
the
lone/test/add-tests-for-non-identity-actors-after-any-actor-request
branch
from
1ceda7a
to
ef4df0a
Compare
| // We can't assume that there is no-access just because there is no identity even if the document | ||
| // is registed with acp, this is because it is possible that acp has a registered relation targeting | ||
| // "*" (any) actor which would mean that even a request without an identity might be able to access | ||
| // a document registered with acp. So we pass an empty `did` to accommodate that case. |
There was a problem hiding this comment.
praise: very descriptive comment
| Policy: ` | ||
| name: Test Policy | ||
|
|
||
| description: A Policy | ||
|
|
||
| actor: | ||
| name: actor | ||
|
|
||
| resources: | ||
| users: | ||
| permissions: | ||
| read: | ||
| expr: owner + reader + writer | ||
|
|
||
| write: | ||
| expr: owner + writer | ||
|
|
||
| nothing: | ||
| expr: dummy | ||
|
|
||
| relations: | ||
| owner: | ||
| types: | ||
| - actor | ||
|
|
||
| reader: | ||
| types: | ||
| - actor | ||
|
|
||
| writer: | ||
| types: | ||
| - actor | ||
|
|
||
| admin: | ||
| manages: | ||
| - reader | ||
| types: | ||
| - actor | ||
|
|
||
| dummy: | ||
| types: | ||
| - actor | ||
| `, |
There was a problem hiding this comment.
nitpick: I almost never read this part. It's just noise in tests
There was a problem hiding this comment.
Haha we have discussed the fixture vs non-fixture multiple times now. Leaving as is.
| testUtils.Request{ | ||
| Identity: testUtils.NoIdentity(), // Can not read without an identity. | ||
|
|
||
| Request: ` | ||
| query { | ||
| Users { | ||
| _docID | ||
| name | ||
| age | ||
| } | ||
| } | ||
| `, | ||
|
|
||
| Results: map[string]any{ | ||
| "Users": []map[string]any{}, // Can't see the documents yet | ||
| }, | ||
| }, | ||
|
|
||
| testUtils.DeleteDoc{ // Since can't read without identity, can't delete either. | ||
| CollectionID: 0, | ||
|
|
||
| Identity: testUtils.NoIdentity(), | ||
|
|
||
| DocID: 0, | ||
|
|
||
| ExpectedError: "document not found or not authorized to access", | ||
| }, | ||
|
|
||
| testUtils.UpdateDoc{ // Since can't read without identity, can't update either. | ||
| CollectionID: 0, | ||
|
|
||
| Identity: testUtils.NoIdentity(), | ||
|
|
||
| DocID: 0, | ||
|
|
||
| Doc: ` | ||
| { | ||
| "name": "Shahzad Lone" | ||
| } | ||
| `, | ||
|
|
||
| SkipLocalUpdateEvent: true, | ||
| }, |
There was a problem hiding this comment.
suggestion: this has been tested already many times. There is no point in doing it every time.
It's better for reading to have the test focused specifically on the relevant actions.
There was a problem hiding this comment.
The one with Identity: testUtils.NoIdentity() not as many times tbh, + this is testing is for specifically with gql.
Even if one or two other one exists, I like that it presents the full story to a new user reading only this test, the state before and after our action we are testing.
|
|
||
| SupportedMutationTypes: immutable.Some( | ||
| []testUtils.MutationType{ | ||
| testUtils.CollectionNamedMutationType, |
There was a problem hiding this comment.
nitpick: might worth also to leave here a small comment
Relevant issue(s)
Resolves #3276
Description
Tasks
How has this been tested?
Specify the platform(s) on which this was tested: