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

Remote Schema Permission Validation is overly strict for enum's with _PLACEHOLDER #10230

Open
Kaelten opened this issue May 10, 2024 · 0 comments
Labels
k/bug Something isn't working

Comments

@Kaelten
Copy link

Kaelten commented May 10, 2024

Version Information

Server Version: v2.34.0

What is the current behaviour?

I have two hasura instances, one acts as the gateway and has a remote schema to the other.

The remote schema has multiple roles of permissions. Some are very small slices of permissions and don't inherit from a larger body of permissions.

This setup has worked fine until we added a new role that just has insert permissions into a table and no update permissions. This left us first with an issue where the remote schema permission validation was erroring because the role we were using for that check didn't have access to those resolvers.

I then tried to use the 'admin' role for the validation query. This started failing for a different reason. On the role specific schemas the on_conflict argument creates empty enums with _PLACEHOLDER as the only value. Again, this validated fine until I tried using admin, and now since admin can see all the values that the enum could have, I get a _PLACEHOLDER not in enum type validation error.

I've traced this back to this snippet:

-- | `validateEnumTypeDefinition` checks the validity of an enum definition
-- provided by the user against the corresponding upstream enum.
-- The function does the following things:
-- 1. Validates the directives (if any)
-- 2. For each enum provided, check if the enum values are a subset of
-- the enum values of the corresponding upstream enum
-- *NOTE*: This function assumes that the `providedEnum` and the `upstreamEnum`
-- have the same name.
validateEnumTypeDefinition ::
(MonadValidate [RoleBasedSchemaValidationError] m) =>
-- | provided enum type definition
G.EnumTypeDefinition ->
-- | upstream enum type definition
G.EnumTypeDefinition ->
m G.EnumTypeDefinition
validateEnumTypeDefinition providedEnum upstreamEnum = do
when (providedName /= upstreamName)
$ dispute
$ pure
$ UnexpectedNonMatchingNames providedName upstreamName Enum
void $ validateDirectives providedDirectives upstreamDirectives G.TSDLENUM $ (Enum, providedName)
for_ (NE.nonEmpty $ S.toList $ duplicates providedEnumValNames) $ \dups -> do
refute $ pure $ DuplicateEnumValues providedName dups
for_ (NE.nonEmpty $ S.toList fieldsDifference) $ \nonExistingEnumVals ->
dispute $ pure $ NonExistingEnumValues providedName nonExistingEnumVals
pure providedEnum
where
G.EnumTypeDefinition _ providedName providedDirectives providedValueDefns = providedEnum
G.EnumTypeDefinition _ upstreamName upstreamDirectives upstreamValueDefns = upstreamEnum
providedEnumValNames = map (G.unEnumValue . G._evdName) $ providedValueDefns
upstreamEnumValNames = map (G.unEnumValue . G._evdName) $ upstreamValueDefns
fieldsDifference = getDifference providedEnumValNames upstreamEnumValNames

What is the expected behaviour?

I should be able to leverage remote schema permissions in this context. To do this I can see two possible paths forward:

  1. Update the schema permission logic to be more permissive by determining if something is a logical subset versus a pure subset, in the case of the enum validation, I believe that could be accomplished by tweaking the logic slightly. While I'm not fluent in haskell, I think this would roughly be the 'fix' by replacing lines 651 and 652 with this:
  for_ (NE.nonEmpty $ S.toList fieldsDifference) $ \nonExistingEnumVals -> do
    when (nonExistingEnumVals /= "_PLACEHOLDER") $
      dispute $ pure $ NonExistingEnumValues providedName nonExistingEnumVals

There could be more spots that would need to be addressed like this, but I'm not aware of them.

  1. Allow for each remote schema permission role to define additional headers that get defined and if they're present do a per-role schema query to get the remote schema for just that query. This would provide a more 'perfect' solution in some ways since the schema would be validated against the exact role that you're testing.

A rough example of what this could look like:

    - role: role_manager
      definition:
        headers:
          x-hasura-role: role_manager
        schema: |
          ...

How to reproduce the issue?

  1. Have a remote schema role with only insert permissions and no update permissions
  2. Try to validate that remote schema permission set when the introspection query uses the admin role.

Any possible solutions/workarounds you're aware of?

Not so far. I've tried:

  • Using an inherited role that would only be used for schema validation, but due to the way inherited roles merge permissions this results in type mismatches.
  • Removing _PLACEHOLDER from the enum definitions, but then the remote schema definition is rejected with a syntax error despite being, I think, technically valid.

Keywords

remote schema, permissions, validation, _PLACEHOLDER

@Kaelten Kaelten added the k/bug Something isn't working label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant