Scopes passed to #find not honored for #update_attributes or #destroy about graphiti HOT 5 CLOSED

Cool, glad we figured it out 🎉 ! You're right - they key here is #base_scope, which applies to all finders including the ones for update/destroy. If the asset belongs to someone else, you'd get an error and 404 response.

The before_save code isn't needed for your specific case I don't think, but more for a general case where maybe only admins are allowed to add new assets. Of course, make sure to test your scenarios regardless.

There might eventually be some work to do in this area, but looks like the original issue is solved. Thanks for your patience @elDub !

from graphiti.

The important thing here is persistence operations should account for sideposting, ie not having an override in the endpoint. If you need this override logic, you probably need it in the Resource itself to account for this, which makes me think #base_scope.

Or am I wrong? What's the scenario where you'd want an override for an endpoint, but not sideposts?

Alternatively, maybe this should be #base_scope_for_writes as opposed to #base_scope?

from graphiti.

Show sequence

Controller

  def show
    contact = MMApi::ContactResource.find(params, current_mm_tenant.contacts)
    respond_to do |format|
      format.jsonapi { render jsonapi: contact }
    end
  end

Database Hits

  MMContact Load (0.3ms)  SELECT  `mm_contacts`.* FROM `mm_contacts` WHERE `mm_contacts`.`tenant_id` = 1 AND `mm_contacts`.`id` = 150 LIMIT 20 OFFSET 0

Destroy sequence

Controller

  def destroy
    contact = MMApi::ContactResource.find(params, current_mm_tenant.contacts)
    if contact.destroy
      render jsonapi: { meta: {} }, status: :ok
    else
      render jsonapi_errors: contact
    end
  end

Database Hits

  MMContact Load (0.2ms)  SELECT  `mm_contacts`.* FROM `mm_contacts` WHERE `mm_contacts`.`id` = 254 LIMIT 20 OFFSET 0
  MMContact Destroy (0.3ms)  DELETE FROM `mm_contacts` WHERE `mm_contacts`.`id` = 254

Note that for the destroy it never verified that the record belonged to the tenant in the query.

I'm trying to avoid writing controller-specific code in the resources like this:

  def base_scope
    resource_scope || model
  end

  def resource_scope
    case context
    when MMApi::ContactsController
      current_mm_tenant.contacts
    end
  end

Perhaps I'm doing something wrong in the context of an API?

from graphiti.

Note that for the destroy it never verified that the record belonged to the tenant in the query.

Wouldn't you want this logic to apply for every action, and in that case use #base_scope?

And if not, would code like this work?

before_save do |model|
  unless context.current_user.can_update?(model) # or whatever
    raise 'not allowed'
  end
end

from graphiti.

You are absolutely correct. It has just taken me too long to understand what you were suggesting and how to code it, but I have come up with the following changes to my resource objects that actually appears to do what I was hoping.

class AssetResource < MMApiResource
  before_attributes do |attrs|
    attrs[:tenant_id] = current_tenant.id
  end

  before_attributes only: :create do |attrs|
    attrs[:added_by_id] = current_user.id
  end

  def base_scope
    current_tenant.assets
  end

  # ...
end

Up until now I was concerned about the base_scope getting applied when this resource was not the root resource being worked with, but it seems to work no matter what.

I believe this code will handle automatically applying the tenant data for new records as well as ensuring that reads and deletes are properly filtered as well. Does this code need the before_save code you proposed, or is that only because I resisted your other methods of handling this for so long?

from graphiti.