Add Go example for TinyTodo based on cedar-go #182

mqf20 · 2024-08-04T14:36:33Z

mqf20 · 2024-08-04T14:42:15Z

Hey @andrewmwells-amazon, I've prepared the PR as suggested.

There seems to be a problem with DCO sign off, do you need me to sign or does it only concern AWS staff?

khieta · 2024-08-05T16:45:38Z

Hi @mqf20! To pass the DCO check you should include -s in your git commit commands. I believe if you click the "Details" link next to the CI failure it'll take you to a page that explains more.

khieta · 2024-08-05T16:50:17Z

(Also: thanks for the contribution! I'll tag some reviewers who have familiarity with Go & the TinyTodo app)

mqf20 · 2024-08-05T17:00:31Z

Got it, I just wasn't sure if the signing policy applies only to AWS staff.

Should I withdraw this PR and resubmit a new one with a signed commit?

cdisselkoen · 2024-08-05T17:16:20Z

You can git commit --amend -s and then force-push to the PR branch

tinytodo-go/entities.json

shaobo-he-aws · 2024-08-05T20:39:24Z

tinytodo-go/test_tinytodo.py

@@ -0,0 +1,76 @@
+from tinytodo import *


I assume that the go version uses the same "frontend" Python file as the Rust version.. If so, we should also use one copy only.

Agreed, although it's not totally clear to me what value the Python frontend adds over just running go build and cmd/server directly. Maybe it lets you interact with the server from a REPL somewhat easily? Dunno.

Yes. We use the Python REPL to interact with the backend server.

shaobo-he-aws · 2024-08-05T20:42:59Z

A high-level feedback from me is to reuse the files in the existing TinyTodo example (e.g., policies, entities, and Python files) as much as possible, which makes me wonder if we should restructure the TinyTodo folder into something like,

tinytodo
  - policy.cedar
  - tinytodo.cedarschema
  - tinytodo.py
  - ...
  - rust
    - Cargo.toml
    - src
  - go
    - ...

mqf20 · 2024-08-06T02:45:44Z

A high-level feedback from me is to reuse the files in the existing TinyTodo example (e.g., policies, entities, and Python files) as much as possible, which makes me wonder if we should restructure the TinyTodo folder into something like,
tinytodo
  - policy.cedar
  - tinytodo.cedarschema
  - tinytodo.py
  - ...
  - rust
    - Cargo.toml
    - src
  - go
    - ...

Agreed, this sounds neater.

However, the Go implementation does not support some features (templates, schemas), so the Rust and Go implementations are not equivalent.

patjakdev

Thank you so much for writing all of this up and adeptly skirting some of the limitations of the current cedar-go implementation!

I mostly skimmed over the actual application logic, but I did look more closely at the interface with cedar-go and had a few questions. I decided to request changes because I think it's pretty important that isAuthorized() adhere to the Cedar ethos of respecting the policy decision, even in the face of errors.

tinytodo-go/entities.json

patjakdev · 2024-08-06T16:39:39Z

tinytodo-go/internal/app/server/context.go

+		for _, e := range diag.Errors {
+			errs = append(errs, errors.New(e.String()))
+		}
+		return false, nil, errors.Join(errs...)


I may be mistaken, but this doesn't seem to match the behavior of the Rust implementation:

let response = self.authorizer.is_authorized(&q, &self.policies, &es); info!("Auth response: {:?}", response); match response.decision() { Decision::Allow => Ok(()), Decision::Deny => Err(Error::AuthDenied(response.diagnostics().clone())), }

That implementation seems to simply log any policy errors and just return the decision, which I think matches the spirit of Cedar's skip-on-error behavior:

The reasoning for the skip-on-error property is more involved. An alternative authorization algorithm we considered would be to Deny a request when any policy evaluation exhibits an error. While this might sound good at first, deny-on-error raises concerns of safety. An application that was working fine with 100 policies might suddenly start denying all requests if the 101st policy has an error. Skip-on-error avoids this dramatic failure mode, and is more flexible: applications can always choose to look at the authorization response’s diagnostics and take a different decision if an evaluated policy produces errors. For more information, see this blog post written by one of the Cedar designers.

You're right, the Go implementation does not match the Rust implementation.

Before I fix the Go implementation, please correct me if I have misinterpreted the Rust implementation (I'm new to Rust):

let response = self.authorizer.is_authorized(&q, &self.policies, &es); info!("Auth response: {:?}", response); match response.decision() { Decision::Allow => Ok(()), Decision::Deny => Err(Error::AuthDenied(response.diagnostics().clone())), }

Returns an empty successful value if the decision is allow

Returns an error with the diagnostics (errors and reasons) if the decision is deny

Based on Cedar's skip-on-error behavior that you quoted, it seems like the Rust implementation has overlooked a scenario: the decision is allow but errors are present.

Based on Cedar's skip-on-error behavior that you quoted, it seems like the Rust implementation has overlooked a scenario: the decision is allow but errors are present.

I think the Rust implementation is following the spirit of the skip-on-error behavior. If the decision is allow, the request should be authorized, even if there are errors. Those errors are logged via the info!() macro above the match statement.

Got it, let me know if this commit is aligned with the Rust implementation

tinytodo-go/internal/app/server/entitystore/entityuid.go

tinytodo-go/internal/app/server/context_test.go

tinytodo-go/internal/app/server/entitystore/list.go

tinytodo-go/tinytodo.py

patjakdev · 2024-08-06T18:13:04Z

tinytodo-go/test_tinytodo.py

@@ -0,0 +1,76 @@
+from tinytodo import *


Agreed, although it's not totally clear to me what value the Python frontend adds over just running go build and cmd/server directly. Maybe it lets you interact with the server from a REPL somewhat easily? Dunno.

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

…y/cedar-examples/pull/182/files#r1705923365) Signed-off-by: mqf20 <mingqingfoo@gmail.com>

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

…-policy#182 (comment)) Signed-off-by: mqf20 <mingqingfoo@gmail.com>

patjakdev

Looks good! Thanks for taking my feedback.

patjakdev · 2024-08-15T23:33:47Z

tinytodo-go/internal/app/server/context.go

@@ -14,18 +15,19 @@ import (
 //
 // Non-existent entities (resources) will result in an error. (TODO: we may not want this behaviour)
 func (s *Server) isAuthorized(
+	ctx context.Context,


nit: I'd consider putting this argument last to match the normal PARC order.

In this case, the context here is different from the context in an authorization request (the C in PARC). In the Rust implementation, the authorization request is also missing.

Perhaps I should make this differentiation clearer?

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

mqf20 · 2024-08-17T06:24:45Z

@patjakdev I just pushed commit 9839aea to clean up the Go module paths, and commit 98ccf5f to remove an irrelevant file I added by accident

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

mqf20 · 2024-08-20T06:35:54Z

@aaronjeline @shaobo-he-aws do you have any comments on this PR?

shaobo-he-aws

LGTM, as a person who rarely knows Go.

aaronjeline

Looks good overall. Two minor requests:

The above comment, if there isn't a clear solution it's fine as is.
Can you adjust the CI to auto-build this?

tinytodo-go/internal/app/server/httphandlersapi.go

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

aaronjeline

Looks good, once CI passes :)

mqf20 changed the base branch from release/3.2.x to main August 4, 2024 14:37

mqf20 mentioned this pull request Aug 4, 2024

TinyTodo example in cedar-go cedar-policy/cedar-go#24

Closed

khieta requested a review from aaronjeline August 5, 2024 16:50

shaobo-he-aws reviewed Aug 5, 2024

View reviewed changes

tinytodo-go/entities.json Show resolved Hide resolved

shaobo-he-aws reviewed Aug 5, 2024

View reviewed changes

patjakdev suggested changes Aug 6, 2024

View reviewed changes

added working tinytodo-go

5ad4eeb

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

mqf20 force-pushed the feature/tinytodo-go branch from d55ec40 to 5ad4eeb Compare August 8, 2024 15:38

mqf20 added a commit to mqf20/cedar-examples that referenced this pull request Aug 12, 2024

fixed error in documentation (see cedar-policy#182 (comment))

b6e4303

mqf20 added 2 commits August 12, 2024 23:15

simplified test helper readFile() (see https://github.com/cedar-polic…

61ab413

…y/cedar-examples/pull/182/files#r1705923365) Signed-off-by: mqf20 <mingqingfoo@gmail.com>

fixed error in documentation (see cedar-policy#182 (comment))

d7a29fa

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

mqf20 force-pushed the feature/tinytodo-go branch from b6e4303 to d7a29fa Compare August 12, 2024 15:16

This was referenced Aug 15, 2024

Restructure tinytodo and tinytodo-go to avoid duplication #185

Open

Improve entities.json in tinytodo to conform to published syntax #186

Open

aligned authorization check behaviour with Rust implementation (cedar…

db4ef4c

…-policy#182 (comment)) Signed-off-by: mqf20 <mingqingfoo@gmail.com>

patjakdev approved these changes Aug 15, 2024

View reviewed changes

mqf20 requested a review from shaobo-he-aws August 17, 2024 05:36

updated go module path

9839aea

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

removed irrelevant file

98ccf5f

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

shaobo-he-aws approved these changes Aug 21, 2024

View reviewed changes

mqf20 requested a review from patjakdev August 22, 2024 03:57

patjakdev approved these changes Aug 22, 2024

View reviewed changes

aaronjeline approved these changes Aug 26, 2024

View reviewed changes

tinytodo-go/internal/app/server/httphandlersapi.go Show resolved Hide resolved

mqf20 added 2 commits August 27, 2024 23:30

updated README

22292b4

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

added ci pipeline for tinytodo-go

a09bbd0

Signed-off-by: mqf20 <mingqingfoo@gmail.com>

mqf20 requested review from aaronjeline and shaobo-he-aws August 29, 2024 17:10

aaronjeline approved these changes Aug 29, 2024

View reviewed changes

aaronjeline merged commit 230f291 into cedar-policy:main Aug 29, 2024
8 checks passed

khieta mentioned this pull request Sep 9, 2024

run TinyTodo Go example in CI #188

Merged

mqf20 deleted the feature/tinytodo-go branch October 3, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Go example for TinyTodo based on cedar-go #182

Add Go example for TinyTodo based on cedar-go #182

mqf20 commented Aug 4, 2024

mqf20 commented Aug 4, 2024

khieta commented Aug 5, 2024

khieta commented Aug 5, 2024

mqf20 commented Aug 5, 2024

cdisselkoen commented Aug 5, 2024

shaobo-he-aws Aug 5, 2024

patjakdev Aug 6, 2024

shaobo-he-aws Aug 13, 2024

shaobo-he-aws commented Aug 5, 2024 •

edited

Loading

mqf20 commented Aug 6, 2024

patjakdev left a comment

patjakdev Aug 6, 2024

mqf20 Aug 11, 2024 •

edited

Loading

patjakdev Aug 14, 2024

mqf20 Aug 15, 2024

patjakdev Aug 6, 2024

patjakdev left a comment

patjakdev Aug 15, 2024

mqf20 Aug 16, 2024 •

edited

Loading

mqf20 commented Aug 17, 2024 •

edited

Loading

mqf20 commented Aug 20, 2024

shaobo-he-aws left a comment

aaronjeline left a comment

aaronjeline left a comment

Add Go example for TinyTodo based on cedar-go #182

Add Go example for TinyTodo based on cedar-go #182

Conversation

mqf20 commented Aug 4, 2024

mqf20 commented Aug 4, 2024

khieta commented Aug 5, 2024

khieta commented Aug 5, 2024

mqf20 commented Aug 5, 2024

cdisselkoen commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaobo-he-aws commented Aug 5, 2024 • edited Loading

mqf20 commented Aug 6, 2024

patjakdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqf20 Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patjakdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqf20 Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

mqf20 commented Aug 17, 2024 • edited Loading

mqf20 commented Aug 20, 2024

shaobo-he-aws left a comment

Choose a reason for hiding this comment

aaronjeline left a comment

Choose a reason for hiding this comment

aaronjeline left a comment

Choose a reason for hiding this comment

shaobo-he-aws commented Aug 5, 2024 •

edited

Loading

mqf20 Aug 11, 2024 •

edited

Loading

mqf20 Aug 16, 2024 •

edited

Loading

mqf20 commented Aug 17, 2024 •

edited

Loading