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

[auth-4300] Correct guard for "Create Rule" button #4317

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

msorens
Copy link
Contributor

@msorens msorens commented Sep 7, 2020

πŸ”© Description: What code changed, and why?

Users who have iam:projects:update permissions were unable to see the Create Rule button on the project details page becuase of this incorrect guard:

<app-authorized [allOf]="['/apis/iam/v2/projects', 'post']">

This PR corrects the guard to this:

<app-authorized [allOf]="['/apis/iam/v2/projects/{project_id}/rules', 'post', project?.id]"

But that is only a small part of the story of this PR. It turns out that just making that guard change did not do the job--in fact, the Create Rule button was not showing up at all! This change exposed that automate-gateway was not supporting this introspection call.
And when I say "this", I mean this single, solitary, unique endpoint. Any other endpoint would have been fine! Go figure...

This could be viewed as either a bug or a feature--take your pick. 😁 We have two endpoints that use the same endpoint path:

ListRulesForProject /apis/iam/v2/projects/{id}/rules
CreateRule /apis/iam/v2/projects/{project_id}/rules

They are equivalent--but not identical: different variable names. If they had used the same variable name, the code would have handled it. And one could argue that they should be the same variable name, but that would introduce a breaking change to the API, so did not seem prudent. The alternative was to revise introspection to now support multiple (i.e. non-identical) entries.

After eliminating the front-end as culpable, I was able to confirm this was a back-end issue.
When introspecting parameterized endpoints, one provides the inflated endpoint in the payload, not the "template". So here it is filled in with project proj1.

BEFORE (this result is incorrect):

$  curl -kH "api-token: $TOK" $TARGET_HOST/apis/iam/v2/introspect?pretty \
    -d "$(jq -n '{ path: "/apis/iam/v2/projects/proj1/rules"}')"
{
  "endpoints": {
    "/apis/iam/v2/projects/{id}/rules": {
      "get": true,
      "put": false,
      "post": false,
      "delete": false,
      "patch": false
    },
    "/apis/iam/v2/projects/{project_id}/rules": {
      "get": false,
      "put": false,
      "post": true,
      "delete": false,
      "patch": false
    }
  }
}

AFTER (this result is correct):

$ curl -kH "api-token: $TOK" $TARGET_HOST/apis/iam/v2/introspect?pretty \
    -d "$(jq -n '{ path: "/apis/iam/v2/projects/proj1/rules"}')"
{
  "endpoints": {
    "/apis/iam/v2/projects/proj1/rules": {
      "get": true,
      "put": false,
      "post": true,
      "delete": false,
      "patch": false
    }
  }
}

Notice that the HTTP methods are supposed to be combined under the single, inflated endpoint (GET and POST are both true), rather than getting the templates back individually with their individual permissions.

Caveat: Since introspection is not yet project-aware, anyone with iam:projects:update permission will see the buttons, regardless of whether they have that permission on a specific project or not.

⛓️ Related Resources

πŸ‘ Definition of Done

image

πŸ‘Ÿ How to Build and Test the Change

In hab studio rebuild components/automate-gateway.

As an admin:

  1. Obtain an admin token and store in the environment variable $ADMINTOK.
  2. Create a proj-update policy:
curl -sSkH "api-token: $ADMINTOK" $TARGET_HOST/apis/iam/v2/policies?pretty -X POST \
-d '{ "name": "proj-update", "id": "proj-update", "members": [ ], "statements": [ { "effect": "ALLOW", "actions": [ "iam:projects:update" ], "projects": [ "*" ] } ], "projects": [] }'
  1. Create a user (e.g., "bob") in the UI and make bob a member of the viewer policy or team.
  2. Create a project (e.g. "proj1").

Login as bob:

  1. Navigate to Settings >> Projects >> proj1 >> Ingest Rules
  2. Observe that there is no Create Rule button on the project details page.
  3. Go back to the Projects page.
  4. On the command line with the admin token, add bob to the proj-update policy:
curl -sSkH "api-token: $ADMINTOK" $TARGET_HOST/apis/iam/v2/policies/proj-update/members:add \
-d '{ "members": [ "user:local:bob" ] }'
  1. Re-open the project details page again.
  2. Observe that now there is a Create Rule button on the project details page.

βœ… Checklist

The code was incorrectly using a guard for the CreateProject endpoint.
I suspect that may have been done as a stop-gap
before <app-authorized> supported parameterized endpoints.

Signed-off-by: michael sorens <msorens@chef.io>
Adding the correct guard (previous commit)
made the "Create Rule" button disappear for everybody!
Tracked down to this feature/bug (take your pick).
We have two endpoints that use the same endpoint path:
```
ListRulesForProject /apis/iam/v2/projects/{id}/rules
CreateRule /apis/iam/v2/projects/{project_id}/rules
```
They are equivalent--but not identical: different variable names.
If they had used the same variable name, the code would have handled it.
But the code did not support multiple (i.e. non-identical) entries.
Now it does.

Signed-off-by: michael sorens <msorens@chef.io>
@netlify
Copy link

netlify bot commented Sep 7, 2020

Deploy preview for chef-automate ready!

Built with commit 5477331

https://deploy-preview-4317--chef-automate.netlify.app

@msorens msorens self-assigned this Sep 7, 2020
@msorens msorens added auth-team anything that needs to be on the auth team board automate-auth labels Sep 7, 2020
@msorens msorens added this to the Auth: Sprint 21 milestone Sep 7, 2020
@msorens msorens requested a review from a team September 7, 2020 03:29
Copy link
Contributor Author

@msorens msorens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author notes.

Comment on lines -133 to -134
// Note: For POST /introspect (this handler), it should be safe to assume
// that there's only ever one endpoint under consideration.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Unless you use different variable names in parameterized endpoints in the proto file! 🀷

&request.IntrospectReq{Path: "/apis/iam/v2/projects/42/rules/509"},
map[string]*response.MethodsAllowed{"/apis/iam/v2/projects/42/rules/509": {Put: true, Delete: true}},
},
"multiple response pairs matching the request with multiple http methods": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the test that confirms the new supported functionality.

@msorens msorens added automate-ui ui customer-reported issues reported by customers labels Sep 7, 2020
@msorens msorens merged commit 1345ca7 into master Sep 8, 2020
@msorens msorens deleted the auth-4300/create-rule-guard branch September 8, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board automate-auth automate-ui customer-reported issues reported by customers ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants