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

[PD-10597] GraphQL Backend Restrictions #57

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

eledumorales
Copy link
Contributor

@eledumorales eledumorales commented Jul 20, 2023

Description

Implements access of the following list:

  1. Queries
  2. Query Fields
  3. Mutations
  4. Mutation Arguments

based on role's restrictions. If restrictions doesn't exist, it will display full data.

Result Format

Queries:
if a certain attribute has a view restriction, it will return null. Required fields can't be restricted because this will cause a schema conflict.

Mutations:
following graphql's result structure, warnings will be added inside { errors: warning: ... } and it will contain a list of the attributes/arguments (key) with the restriction (value). In the following example, it displays Project restrictions on name attribute and also restrictions on User's teamId attribute, which is a Project Texter's nested resource:

"errors": {
        "warning": {
          "project": [
            {
              "name": "don't have permissions to update"
            },
            {
              "projectTexter": [
                {
                  "user": [
                    {
                      "teamId": "don't have permissions to create"
                    }
                  ]
                }
              ]
            }
          ]
        }
      }

obs:

  • Required fields can't be restricted for queries and mutations because of schema conflicts.
  • Specify restriction type warning for nested attributes will be added in another ticket

Checklist

  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • All status checks (tests, linting, etc...) are passing.

@kylejginavan
Copy link
Member

expect(data["createAdvisor"]["resource"]["nickname"]).to be_nil
expect(data["createAdvisor"]["resource"]["optionalOrg"]).to be_nil
expect(data["createAdvisor"]["errors"]["warning"]["advisor"].size).to eql 2
expect(Advisor.where(name: name).exists?).to eql true

Choose a reason for hiding this comment

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

why was the advisor created if the user had restrictions on those fields? Or was the advisor created but the fields that were restricted just were filled with nil ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the second description is the correct one, advisor will be created but with restricted attributes filtered. User will receive a warning about the attributes that were not saved because of role's restrictions

restriction = ctx[:current_user]&.restrictions&.detect do |el|
(el.resource.name == name || el.resource.alias == name) &&
el.restriction_operation_id == "HasHelpers::RestrictionOperation::::View" &&
el.resource.resource_type_id != "HasHelpers::ResourceType::::RequiredField"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to check RequiredField to view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

required fields can't be restricted because this will cause a schema conflict. For example if attribute name is required, the schema assures that this attribute will always be returned as string. If it has a restriction, it will try to return null, causing a query fail for schema error

@kylejginavan
Copy link
Member

kylejginavan commented Aug 28, 2023

  1. Lots of duplicate logic. I think there should be a helper method that passes base resource name and field name to a helper function to do the checks.
  2. I don't think the specs cover all the use cases.

(el.resource.resource_type_id == "HasHelpers::ResourceType::::BaseResource" &&
restriction_operations.include?(el.restriction_operation_id))
end
restrictions
Copy link
Member

@kylejginavan kylejginavan Aug 28, 2023

Choose a reason for hiding this comment

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

Why are we using ruby here instead of sql? I would add specs then convert to SQL. Same comment applies to many of the functions in this file.

# restriction_operations is an array of ::HasHelpers::RestrictionOperation
def get_base_restrictions(restriction_operations)
restrictions = context[:current_user]&.restrictions&.select do |el|
(el.resource.resource_type_id == "HasHelpers::ResourceType::::BaseResource" &&
Copy link

Choose a reason for hiding this comment

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

memoize base_restrictions thus is always the same

@base_restrictions ||=  context[:current_user]&.restrictions&.select do |el|
     (el.resource.resource_type_id == "HasHelpers::ResourceType::::BaseResource")
end
restrictions = @base_restrictions.select { |r|  restriction_operations.include?(r.restriction_operation_id) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants