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

Extracting the loading of source and related resources in controller actions #108

Closed
barelyknown opened this issue Mar 10, 2015 · 4 comments

Comments

@barelyknown
Copy link
Collaborator

I'd like to ensure that a user is authorized to view a related resource before it is rendered, but there's no way currently without completely reimplementing the get_related_resource method.

Here's the current implementation of get_related_resource in JSONAPI::ResourceController:

def get_related_resource
  association_type = params[:association]
  source_resource = @request.source_klass.find_by_key(@request.source_id, context: context)

  serializer = JSONAPI::ResourceSerializer.new(@request.source_klass,
                                               include: @request.include,
                                               fields: @request.fields,
                                               base_url: base_url,
                                               key_formatter: key_formatter,
                                               route_formatter: route_formatter)

  render json: serializer.serialize_to_hash(source_resource.send(association_type))
end

Ideally, we would extract two methods that could be overridden without needing to completely reimplement this method. Something like:

def get_related_resource
  source_resource = load_source_resource(@request)
  related_resource = load_related_resource(source_resource, params[:association])
  serializer = JSONAPI::ResourceSerializer.new(@request.source_klass,
                                               include: @request.include,
                                               fields: @request.fields,
                                               base_url: base_url,
                                               key_formatter: key_formatter,
                                               route_formatter: route_formatter)

  render json: serializer.serialize_to_hash(related_resource)
end

def load_source_resource(request)
  # ...
end

def load_related_resource(source_resource, association_name)
  # ...
end

With these methods extracted, users of the library would have the option of authorizing the source and related resources before they're rendered. Something like:

def load_related_resource(source_resource, association_name)
  related_resource = source_resource.public_send(association_name)
  unless authorized?(context[:current_user], related_resource.model)
    raise NotAuthorizedError
  end
  related_resource
end

I could also imagine handling this requirement by adding a new callback (before_render maybe?). But I think that I prefer the method that I mentioned above.

For what it's worth, a similar problem exists with other controller actions too (some can be handled with the existing callbacks, some can't). We should use similar approaches for each action.

I'd be happy to write a PR for this tomorrow, but I wanted to discuss this issue and approach before adding the feature.

@lgebhardt
Copy link
Member

@barelyknown would it be possible for you to use the records method on the resource to apply the permission check? That's how I'm doing things in my code and it seems to work well. We're taking the approach that if a user doesn't have permission to see a record then they get a 404, even if the record exists. That may not work for you of course. But it does keep the logic more tightly confined to the resource.

If that doesn't work for you I'm not at all opposed to splitting up the controller methods as you proposed. It could DRY it up a bit either way.

@barelyknown
Copy link
Collaborator Author

I'm using the same approach, but that doesn't work for the get_related_resource action because the records method loads the source resource, not the related resource(s).

@lgebhardt
Copy link
Member

Good catch! In Resource._associate we could add a method similar to records for each association. Then the same approach could be used. I think that could work, but my mind's a bit tired now, so no promises.

@barelyknown
Copy link
Collaborator Author

I was tired last night too!, but I thought about it more this morning and liked your suggestion. The PR implements that approach.

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

No branches or pull requests

2 participants