venuu / jsonapi-authorization

Authorization for JSONAPI::Resource
http://www.rubydoc.info/github/venuu/jsonapi-authorization
MIT License
132 stars 59 forks source link

Check for authorization against related records #119

Closed brianswko closed 5 years ago

brianswko commented 5 years ago

Fixes https://github.com/venuu/jsonapi-authorization/issues/118

This PR fixes a bug where related resources are not checked for authorization if they are not in the user's scope.

Also fixes inconsistencies between the behavior of has-one, has-many, and polymorphic relationships to always return a 404 instead of sometimes returning a 403 when the user should not be able to see that a given record exists.

valscion commented 5 years ago

Sorry about Travis CI failing ...again... we've fixed the errors caused by bundler upgrade several times now but it seems they keep on failing.

valscion commented 5 years ago

I fixed the CI in https://github.com/venuu/jsonapi-authorization/pull/120 and merged master here. Let's see what the CI says now.

valscion commented 5 years ago

CI seems to be happy. And so am I :relaxed:

This seems like it's ready to go, right? How would we describe this change, similarly to how we've done change docs in the past? https://github.com/venuu/jsonapi-authorization/releases

valscion commented 5 years ago

@all-contributors please add @brianswko for bug, test and code

Let me know if you'd rather not be listed in the README. I'll wait for your confirmaton before merging the README.md update by the bot.

allcontributors[bot] commented 5 years ago

@valscion

I've put up a pull request to add @brianswko! :tada:

brianswko commented 5 years ago

Proposed description:

Fixes PATCH and POST requests to check if the user has the correct permissions for every given object in a has-many relationship

For example: If a user does not have access to (meaning the pundit scope does not include) the author with ID 2 i.e. AuthorPolicy::Scope.new(user, Author).resolve.include?(Author.find(2)) # => false And the following request is called:

PATCH /books/1

"data": {
  "type": "books",
  "id": "1",
  "attributes": {...},
  "relationships": {
    "authors": {
      "data": [
        { "type": "authors", "id": "1" },
        { "type": "authors", "id": "2" }
      ]
    }
  }
}

Previously: Would return a 20x and update the book to include author 2 Now: Will return a 404 and not update the book since the user is unable to find author 2

In some scenarios, this will cause a 404 to be returned where a 403 used to be returned.

brianswko commented 5 years ago

@valscion I've simplified the change in the processor, but ended up having to make a decent amount of changes to the specs. Hoping those all make sense to you.

I'm conflicted as to whether this should be a major or minor version change, as there will almost certainly be a not-insignificant number of projects that will start seeing specs fail as bugs in specs are exposed and formerly incorrect 403s start returning 404s. Will leave that decision in your hands.

valscion commented 5 years ago

https://github.com/venuu/jsonapi-authorization/pull/119#issuecomment-476550980

Fixes PATCH and POST requests to check if the user has the correct permissions for every given object in a has-many relationship

Nice. This entire change description is great.

I'm conflicted as to whether this should be a major or minor version change, as there will almost certainly be a not-insignificant number of projects that will start seeing specs fail as bugs in specs are exposed and formerly incorrect 403s start returning 404s. Will leave that decision in your hands.

Given we only recently upgraded to 2.0.0 to fix an authorization bug, and only less than 1000 downloads as of this date have been done against v2.0.0, we could do this change and just bump to 3.0.0.

I'd rather signal a potentially backwards incompatible change with more major versions than to squint my eyes and think this could be a backwards-compatible change.

So once this is ready to be merged, let's punt this to 3.0.0.

valscion commented 5 years ago

This looks like it's ready to go for me! What do you think? Is it ready?

I'll update the README to clarify what's the intent on version numbering here: https://github.com/venuu/jsonapi-authorization/pull/123. I think it's likely to be helpful given how this change will be shipped as v3.0.0 once we merge it.

I'll copy the text of https://github.com/venuu/jsonapi-authorization/pull/119#issuecomment-476550980 for the release notes. Let me know if there's any changes you'd like there to be.

brianswko commented 5 years ago

Yes it's ready to go, and that comment works for the changelog.

valscion commented 5 years ago

Thank you! This has now been released as v3.0.0 :tada: