Skip to content

Validate requests #11

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

Closed
wants to merge 5 commits into from
Closed

Validate requests #11

wants to merge 5 commits into from

Conversation

khieta
Copy link
Contributor

@khieta khieta commented Jun 23, 2023

rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@khieta khieta mentioned this pull request Jun 23, 2023
@khieta
Copy link
Contributor Author

khieta commented Jun 27, 2023

I've revised the RFC and am looking for new reviews. Summary of changes:

  1. Revised the User/File/Folder example per Mike's suggestion.
  2. Added an additional example to the motivation section to show why validating requests is important in conjunction with standard validation.
  3. Added an "unresolved question" about changing the Request::new API vs. adding a new Request::new_with_validation API.

Please weigh in on point (3) if you haven't already.

@khieta
Copy link
Contributor Author

khieta commented Jun 30, 2023

Revised! Notable changes:

  • Per in-person discussion yesterday, instead of modifying the Request::new constructor, we will add a new Request::new_with_validation constructor.
  • Added more concrete details about implementation (copied below for reference).

The new API will have the following signature:

pub fn new_with_validation(
        principal: Option<EntityUid>,
        action: Option<EntityUid>,
        resource: Option<EntityUid>,
        context: Context,
        schema: &Schema
    ) -> Result<Request> {
        ...
    }

(This is the same as the signature for Request::new, aside from the schema argument and return type.)

If the action is None (indicating that it is "unspecified"), then the function performs no additional checks. If the action is Some, then the function checks that the specified action is present in the schema, and that the principal and resource are consistent with the action's appliesTo lists in the schema.

@cdisselkoen cdisselkoen added the pending This RFC is pending; for definitions see the README label Jul 3, 2023
@anwarmamat anwarmamat self-requested a review July 11, 2023 13:44
@khieta
Copy link
Contributor Author

khieta commented Jul 18, 2023

Based on discussion, we generally support the addition of the Request::new_with_validation constructor. However, I've decided that this change does not merit the bar for an RFC, so rather than moving into FCP, I will be closing the issue. The core content of the RFC will be moved to an issue in the cedar repository.

@khieta khieta closed this Jul 18, 2023
@khieta khieta removed the pending This RFC is pending; for definitions see the README label Jul 18, 2023
@khieta
Copy link
Contributor Author

khieta commented Jul 18, 2023

Moved to cedar issue #191.

@khieta khieta deleted the rfc/khieta/validate-queries branch July 18, 2023 18:54
@john-h-kastner-aws john-h-kastner-aws added the moved-to-issue This proposal does not necessitate RFC and was converted to an issue label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
moved-to-issue This proposal does not necessitate RFC and was converted to an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants