wp-graphql / wp-graphql-acf

WPGraphQL for Advanced Custom Fields
https://wpgraphql.com/acf
626 stars 123 forks source link

Relationship field crashes when referencing trashed posts as anonymous #167

Closed esamattis closed 3 years ago

esamattis commented 4 years ago

To reproduce

{
  page(idType: URI, id: "/test-page") {
    myFieldGroup {
      contacts {
        ... on Contact {
          title
          uri
        }
      }
    }
  }
}

Here's the myFieldGroup is an ACF field group and contacts is the custom post type.

When running this from the graphiql in wp-admin it works but when sending it as an anonymous query it errors with with:

Abstract type Page_MyFieldGroup_Contacts must resolve to an Object type at runtime for field Page_MyFieldGroup.contacts with value \"instance of WPGraphQL\\Model\\Post\", received \"null\". Either the Page_MyFieldGroup_Contacts type should provide a \"resolveType\" function or each possible type should provide an \"isTypeOf\" function.

My initial investigation leads here:

https://github.com/wp-graphql/wp-graphql-acf/blob/01577deb0a474f5387d60d3680a69ff810bddf35/src/class-config.php#L520

The issue goes away if I add $post_object->post_status === 'publish' check for anonymous users before appending to the $relationship array but I'm not sure this is the right place to do it.

Thoughts?

I can also provide better reproduce steps with code if needed.

PSalaun commented 3 years ago

Hi,

I had the same issue, with trashed posts and now with private ones as well. Your fix does work perfectly; you should create a PR, so it'll have more attention from maintainers!

esamattis commented 3 years ago

@PSalaun I'd need some input from @jasonbahl since I suspect this might not be the right place to do it

esamattis commented 3 years ago

Oh, and here's a filter based workaround. Use with caution.

// Workaround for https://github.com/wp-graphql/wp-graphql-acf/issues/167
add_filter(
    'acf/load_value',
    function ($post_ids, $parent_id, $field) {
        if (!function_exists('is_graphql_http_request')) {
            return $post_ids;
        }

        if (!is_graphql_http_request()) {
            return $post_ids;
        }

        // No need to filter this if the viever has the enough permissions
        if (current_user_can('edit_posts')) {
            return $post_ids;
        }

        $type = $field['type'] ?? null;

        if ('post_object' === $type) {
            $post = get_post($post_ids);
            if ($post->post_status === 'publish') {
                return $post_ids;
            } else {
                return null;
            }
        }

        // Affects only the 'relationship' field types
        if ('relationship' !== $type) {
            return $post_ids;
        }

        if (!is_array($post_ids)) {
            return $post_ids;
        }

        $public_ids = [];

        foreach ($post_ids as $id) {
            $post = get_post($id);
            if ($post->post_status === 'publish') {
                $public_ids[] = $post->ID;
            }
        }

        return $public_ids;
    },
    10,
    3
);
huygaa11 commented 3 years ago

This workaround helps. Small improvement would be to use "acf/load_value/type=relationship" and remove the $type variable.

esamattis commented 3 years ago

This issue happens on post_object fields too. I updated the above workaround to include it.

jasonbahl commented 3 years ago

The upcoming v0.6.0 (#262) fixes this.

Relationship fields, such as this one, have been changed to connections and make use of the underlying GraphQL Loader / Model layer to make sure data is loaded properly and only returned to users that have permission to see the data.

This happens at a more central place so that we don't have to do things like check user capabilities on every field, we can check it centrally on the Model (the Post, or other object) that is being requested.

You can read more about this here as well: https://github.com/wp-graphql/wp-graphql-acf/issues/173#issuecomment-840677785

upham-ui commented 2 years ago

We had trouble with the fix above; it failed to iterate over an array of post IDs. Here's what we're using:

add_filter(
  'acf/load_value/type=relationship',
  function ($post_ids, $parent_id, $field) {
    if (!function_exists('is_graphql_http_request')) {
      return $post_ids;
    }

    if (!is_graphql_http_request()) {
      return $post_ids;
    }

    // No need to filter this if the viever has the enough permissions
    if (current_user_can('edit_posts')) {
      return $post_ids;
    }

    if (!is_array($post_ids)) {
      return $post_ids;
    }

    $public_ids = [];

    foreach ($post_ids as $id) {
      $post = get_post($id);
      if (!empty($post) && $post->post_status === 'publish') {
        $public_ids[] = $post->ID;
      }
    }

    return $public_ids;
  },
  10,
  3
);
rotijoe commented 2 years ago

I'm sorry for the newbie question, but where do you place the work around? Functions.php, or somewhere within the plugin code?

esamattis commented 2 years ago

functions.php works. Although this issue should be resolved and the workarounds should not be needed anymore.

CallumMitchellRV commented 2 years ago

@jasonbahl I appreciate the fix for this coming in v0.6.0. As a stop-gap, I've implemented the same changes on my local plugin. I've noticed this same logic needs applying to post object fields. Relationship fields are validating post visibility, but post object fields aren't. To prevent potentially duplicating a ticket, has this been raised elsewhere? Thanks!

maximilliangeorge commented 1 year ago

I've noticed this same logic needs applying to post object fields. Relationship fields are validating post visibility, but post object fields aren't.

I can confirm this is still an issue in WPGraphQL for ACF 0.6.1.