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

Keto expand REST API does not parse query parameter max-depth correctly #704

Closed
counter2015 opened this issue Sep 10, 2021 · 4 comments · Fixed by #755
Closed

Keto expand REST API does not parse query parameter max-depth correctly #704

counter2015 opened this issue Sep 10, 2021 · 4 comments · Fixed by #755
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.

Comments

@counter2015
Copy link

Describe the bug

Keto exapnd REST API will return http code 400 when there is not max-depth in query parameter.

Reproducing the bug
image

func (h *handler) getExpand(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
depth, err := strconv.ParseInt(r.URL.Query().Get("max-depth"), 0, 0)
if err != nil {
h.d.Writer().WriteError(w, r, herodot.ErrBadRequest.WithError(err.Error()))
return
}

Expected behavior

It should behave as doc says that max-depth is not required.

Or it should be consistent with cli behavior, that's to say, has a default value

$ keto expand grant role company-admin

∪ role:company-admin#grant
├─ ☘ role:super-admin️
├─ ∪ role:super-admin#member
│  ├─ ☘ user:ZhangThree️

Environment

  • Version: 0.6.0-alpha.3
  • Environment: Docker compose, PostMan
@counter2015 counter2015 changed the title Keto expand REST API does not parse query parameter max-depth Keto expand REST API does not parse query parameter max-depth correctly Sep 10, 2021
@zepatrik
Copy link
Member

True, as it is a query parameter it should not be required but come with a default. Should have the same default value everywhere, so CLI, REST handler, and gRPC handler.

@zepatrik zepatrik 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 Sep 13, 2021
@Lumexralph
Copy link

@zepatrik I would also like to look into this issue but not until the other issue I am working on is resolved.

@AshirwadPradhan
Copy link

Hi, I would like to work on this issue. Is there anyone else working on it ?

CypherpunkSamurai added a commit to TaishoVault/keto that referenced this issue Oct 5, 2021
@CypherpunkSamurai
Copy link
Contributor

CypherpunkSamurai commented Oct 5, 2021

I have added a PR #755 that hopefully solves this issue. max-depth is now required as confirmed by @zepatrik

The strconv.ParseInt() function call on line 78 recieved null string from url.Query().Get(). This caused exception that was also handled as any other error.

This was an unintended error but worked just as required 😆

I made few changes that clean it for better understanding.

Happy Hacktoberfest to Everyone 😄

CypherpunkSamurai added a commit to TaishoVault/keto that referenced this issue Oct 6, 2021
Fix ory#704 `max-depth` is not an optional parameter. Added error handlers, docs and tests(expand_tests.go#L38). Fix sdkClient maxDepth pointer
CypherpunkSamurai added a commit to TaishoVault/keto that referenced this issue Oct 6, 2021
Fix ory#704  is not an optional parameter. Added error handlers,(handler.go#L80) docs and tests(ehandler_tests.go#L38). Fix sdkClient maxDepth pointer. Rebased Commit history.
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
Development

Successfully merging a pull request may close this issue.

5 participants