fix(cli): distinguish between subjectSet and subjectID in check command #985
Conversation
|
|
|
Hi, and thanks for your contribution! It looks quite solid already, but could you add some tests to see that it actually works? Thanks! |
cmd/check/root.go
Outdated
| var s *rts.Subject | ||
|
|
||
| // distinguish between subjectSet and subjectID | ||
| if strings.Contains(subject, "#") { |
There was a problem hiding this comment.
I'd rather distinguish based on : here, since the relation could be empty (= wildcard) for a subject set, e.g., check users:Foo access files README.md.
Hi, Yes I'll do :) |
ketoapi/enc_proto.go
Outdated
| @@ -57,6 +57,15 @@ func (r *RelationTuple) ToProto() *rts.RelationTuple { | |||
| return res | |||
| } | |||
|
|
|||
| func (s *SubjectSet) ToProto() *rts.SubjectSet { | |||
There was a problem hiding this comment.
It seems this is not used? I'd recommend to remove it again then 😉
internal/e2e/cases_test.go
Outdated
| for _, subjectID := range subjects { | ||
| c.createTuple(t, &ketoapi.RelationTuple{ | ||
| Namespace: n.Name, | ||
| Object: obj, | ||
| Relation: rel, | ||
| SubjectID: &subjectID, | ||
| }) | ||
| } | ||
|
|
||
| ss := &ketoapi.SubjectSet{ | ||
| Namespace: n.Name, | ||
| Object: obj, | ||
| Relation: rel, | ||
| } | ||
| rt := &ketoapi.RelationTuple{ | ||
| Object: obj, | ||
| SubjectSet: ss, | ||
| Namespace: n.Name, | ||
| Relation: rel, | ||
| } | ||
|
|
||
| assert.True(t, c.check(t, rt)) |
There was a problem hiding this comment.
The reason why this test is failing is that the created tuples are not related to the checked tuple. It would be sufficient here to just create one tuple with a subject set and then check for the same tuple.
cmd/check/root.go
Outdated
| var s *rts.Subject | ||
|
|
||
| // distinguish between subjectSet and subjectID | ||
| if strings.Contains(subject, ":") { | ||
| su := &ketoapi.SubjectSet{} | ||
| su, err := su.FromString(subject) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| s = rts.NewSubjectSet(su.Namespace, su.Object, su.Relation) | ||
| } else { | ||
| s = rts.NewSubjectID(subject) | ||
| } |
There was a problem hiding this comment.
I'd prefer to make this a small helper function that has its own unit test. That way we can specifically test for the subject parsing of the CLI with more edge cases. The e2e test makes sense as it is right now as well just to ensure that subject sets are parsed and supported by all clients.
attempt of fixing #850 but the command:
./keto check 'videos:/cats/1.mp4#owner' view videos '/cats/1.mp4'still Outputs "Denied" even though it should be "Allowed" because of: