Check for authorization against related records by brianswko · Pull Request #119 · venuu/jsonapi-authorization

brianswko · 2019-03-21T05:15:09Z

Fixes #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.

…cope

valscion · 2019-03-21T07:30:21Z

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 · 2019-03-21T07:43:35Z

I fixed the CI in #120 and merged master here. Let's see what the CI says now.

valscion · 2019-03-21T07:49:51Z

CI seems to be happy. And so am I ☺️

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 · 2019-03-21T07:51:21Z

@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 · 2019-03-21T07:51:29Z

@valscion

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

brianswko · 2019-03-21T18:59:41Z

spec/requests/tricky_operations_spec.rb

+      context 'which is out of scope' do
+        let(:policy_scope) { Article.none }
+
+        it { is_expected.to be_forbidden }


@valscion I just realized while testing that instead of being forbidden this should probably be a not found response. I'm going to rework the processor a bit.

Super ☺️. Take your time

brianswko · 2019-03-26T09:53:33Z

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 · 2019-03-26T10:03:07Z

@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 · 2019-03-26T10:54:25Z

spec/requests/included_resources_spec.rb

+    end
+    allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) do |policy_scope|
+      user_policy_scope.merge(policy_scope.scope.all)
+    end


I'm having a bit of a hard time figuring out what these policy scope changes do. It seems like it's overriding the policy scope stubbing to always return all results?

Maybe there could be some sort of a documentation comment here to tell why this is necessary.

I wanted to get rid of these entirely, but that required a much larger overhaul of the tests than I wanted to take on.

As previously written, this spec never checked which has_many records were returned except in situations where the spec stubbed the comments_policy_scope to be Comment.limit(1), and it masked a fundamental problem with the mock when I added additional scenarios to test.

jsonapi-authorization/lib/jsonapi/authorization/pundit_scoped_resource.rb

Lines 15 to 28 in 05785ff

def records_for(association_name)

record_or_records = @model.public_send(association_name)

relationship = fetch_relationship(association_name)

case relationship

when JSONAPI::Relationship::ToOne

record_or_records

when JSONAPI::Relationship::ToMany

user_context = JSONAPI::Authorization.configuration.user_context(context)

::Pundit.policy_scope!(user_context, record_or_records)

else

raise "Unknown relationship type #{relationship.inspect}"

end

end

The value of policy_scope! is supposed to be the records that the user has access to, further restricted by the specific record_or_records in the association. A simple and_return like what was here before would return all records in the user's scope, even if it had nothing to do with the @model under test. For example, a user who has access to all articles and looked up the articles written by a single user would get a list of all articles instead of just the articles written by that user.

merge acts like a sql AND, so this change allows the stubbed scope to return something more similar to how it should actually behave outside of tests.

Will add a comment for this.

Perfect, thanks for clarifying!

valscion · 2019-03-26T10:55:38Z

spec/requests/included_resources_spec.rb

-          expect(json_included.length).to eq(1)
-          expect(json_included.first["id"]).to eq(comments_policy_scope.first.id.to_s)
+        it 'includes only comments allowed by policy scope and associated with the article' do
+          expect(json_included.length).to eq(article.comments.count)


Hmm so does this now test that there are more records than were allowed? I.e. do we lose test coverage on the pundit policy scope being applied?

I moved the specs specifically testing for limited scopes to https://github.com/venuu/jsonapi-authorization/pull/119/files#diff-f3a557fdd906ce389d7b5d190c721b99R220. I had to move them out of this shared spec since the behavior is different for GET (which returns the relationships but limited by the scope) vs POST (which should return a 404).

Nice, thanks!

lib/jsonapi/authorization/authorizing_processor.rb

valscion · 2019-03-26T10:59:47Z

#119 (comment)

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 · 2019-03-27T07:20:38Z

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: #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 #119 (comment) for the release notes. Let me know if there's any changes you'd like there to be.

brianswko · 2019-03-27T17:35:04Z

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

valscion · 2019-03-28T10:44:23Z

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

* AuthorizingProcessor#related_models_with_context should not use the scope * Remove unused AuthorizingProcessor#related_models * Check all related records in authorize_remove_to_many_relationships * Return 404 instead of 403 for records out of scope * Style fixes * Optimization for Array#size * Comment to clarify changes to mocks * Better array matching in specs

This reverts commit 065a623.

brianswko added 3 commits March 20, 2019 20:59

AuthorizingProcessor#related_models_with_context should not use the s…

38070a2

…cope

Remove unused AuthorizingProcessor#related_models

4463be5

Check all related records in authorize_remove_to_many_relationships

c01a193

Merge branch 'master' into check-related-on-update

5b01e99

allcontributors bot mentioned this pull request Mar 21, 2019

docs: add brianswko as a contributor #121

Closed

venuu deleted a comment from allcontributors bot Mar 21, 2019

allcontributors bot mentioned this pull request Mar 21, 2019

docs: add brianswko as a contributor #122

Merged

brianswko commented Mar 21, 2019

View reviewed changes

brianswko added 2 commits March 26, 2019 00:35

Return 404 instead of 403 for records out of scope

207a439

Style fixes

1340f9c

valscion reviewed Mar 26, 2019

View reviewed changes

lib/jsonapi/authorization/authorizing_processor.rb Outdated Show resolved Hide resolved

brianswko added 3 commits March 26, 2019 12:50

Optimization for Array#size

e9552c2

Comment to clarify changes to mocks

0535a61

Better array matching in specs

bf3a859

valscion mentioned this pull request Mar 27, 2019

Clarify versioning and link to changelogs in README.md #123

Merged

valscion approved these changes Mar 27, 2019

View reviewed changes

valscion merged commit 0708aaf into venuu:master Mar 28, 2019

brianswko mentioned this pull request Mar 29, 2019

Fix a bug with has-one relationship authorizers #124

Merged

brianswko added a commit to brianswko/jsonapi-authorization that referenced this pull request Oct 2, 2019

Revert "Check for authorization against related records (venuu#119)"

5563521

This reverts commit 065a623.

	def records_for(association_name)
	record_or_records = @model.public_send(association_name)
	relationship = fetch_relationship(association_name)

	case relationship
	when JSONAPI::Relationship::ToOne
	record_or_records
	when JSONAPI::Relationship::ToMany
	user_context = JSONAPI::Authorization.configuration.user_context(context)
	::Pundit.policy_scope!(user_context, record_or_records)
	else
	raise "Unknown relationship type #{relationship.inspect}"
	end
	end

Conversation

brianswko commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Mar 21, 2019

Uh oh!

valscion commented Mar 21, 2019

Uh oh!

valscion commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Mar 21, 2019

Uh oh!

allcontributors bot commented Mar 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianswko commented Mar 26, 2019

Uh oh!

brianswko commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valscion commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Mar 27, 2019

Uh oh!

brianswko commented Mar 27, 2019

Uh oh!

valscion commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

brianswko commented Mar 21, 2019 •

edited

Loading

valscion commented Mar 21, 2019 •

edited

Loading

valscion commented Mar 26, 2019 •

edited

Loading

valscion commented Mar 28, 2019 •

edited

Loading