Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policies and roles list should return empty array if list is empty #217

Closed
alekitto opened this issue Jul 2, 2020 · 1 comment · Fixed by #250
Closed

Policies and roles list should return empty array if list is empty #217

alekitto opened this issue Jul 2, 2020 · 1 comment · Fixed by #250
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Milestone

Comments

@alekitto
Copy link

alekitto commented Jul 2, 2020

Describe the bug

Policies and roles list are returning null instead of empty list if all elements are filtered out.
This breaks api contract and generates an error in php client (for example)

Reproducing the bug

Steps to reproduce the behavior:

  1. Send a GET request to /engines/acp/ory/glob/policies on a newly created keto server
  2. Response is null instead of []
@aeneasr aeneasr added bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Aug 22, 2020
@aeneasr aeneasr added this to the unplanned milestone Aug 22, 2020
@xlanor
Copy link
Contributor

xlanor commented Oct 1, 2020

I've been tracing this bug because it also affects us at work, and I've traced it to this
func (h *Handler) List(factory func(context.Context, *http.Request, httprouter.Params) (*ListRequest, error)) httprouter.Handle in storage/handler.go returns a h.h.Write(w, r, l.Filter(m).Value), and in this case, l.Filter(m).Value returns *storage.Policies, which is declared as a type of []Policy .

h, in this case, which is a type of herodot.Writer, an interface, in this particular function, is a herodot.JSONWriter, and Write in this case calls WriteCode here. This tells me that there's an issue with encoding/json's json.Marshal(e), where e refers to l.Filter(m).Value.

I believe this is due to golang/go#27589, and a proposal has been made to modify encoding/json that is still under going review here and awaiting a PR at golang/go#27813.

At this point, I'm wondering if I should submit a PR to ory/herodot, to do the checking in func WriteCode. If it is a shared library, do you think that this will affect other projects that ory has?

There's a quick fix, as can be seen from CosmWasm/wasmvm#75, to quickly check the value, and if it's nil, do not even proceed with JSON.Marshal. This might actually be the fastest way to fix this, but I'm unsure about the potential impact, given that herodot is a shared repository.

Without touching herodot and remaining in keto, I was able to fix this issue by changing the following:

func ListByQuery(l *ListRequest, m map[string][]string) {
	switch val := l.Value.(type) {
	case *Roles:
		var res Roles
		for _, role := range *val {
			filteredRole := role.withMembers(m["member"]).withIDs(m["id"])
			if filteredRole != nil {
				res = append(res, *filteredRole)
			}
		}
		l.Value = &res
	case *Policies:
		res := make([] Policy, 0) // changed this from var res Policies
		for _, policy := range *val {
			filteredPolicy := policy.withSubjects(m["subject"]).withResources(m["resource"]).withActions(m["action"]).withIDs(m["id"])
			if filteredPolicy != nil {
				res = append(res, *filteredPolicy)
			}
		}

		l.Value = &res

	case *[]Policy:           // Added a new case here, because it seems like the first call of this is of Policies type, which I can't seem to find where it is being called.
		res := make([]Policy, 0)
		for _, policy := range *val {
			filteredPolicy := policy.withSubjects(m["subject"]).withResources(m["resource"]).withActions(m["action"]).withIDs(m["id"])
			if filteredPolicy != nil {
				res = append(res, *filteredPolicy)
			}
		}

		l.Value = &res
	default:
		panic("storage:unable to cast list request to a known type!")
	}
}

but it doesn't look very clean.

@aeneasr , do you have any suggestions? I can work on a PR this week.

EDIT: Found the Policies call. I will work on a PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
3 participants