fix(gravsearch): Unify permissions handling with full resource request #1521

benjamingeer · 2019-11-20T10:47:38Z

This PR makes Gravsearch filter results according to user permissions in the way that the /v2/resources route does, as per https://discuss.dasch.swiss/t/change-in-the-way-gravsearch-results-are-filtered-according-to-permissions/111.

Fixes #1344
Closes #924

mrivoal

I can only approve this PR on the principle, by reading the comments and not the code itself :)

There is a use case I would like to draw your attention to: given the fact that Default Access Object Permission can be defined at property level as well as at resource level, a permission schema like the one that follows is possible :

define a restricted Default Access Object Permission on a resource
define a more open Default Access Object Permission on the link property that targets this resource.

For example : given 2 resource classes, Person and Biography, Person having a link property hasBiography on Biography, what do you think would happen if the permissions were set like this:

<http://rdfh.ch/permissions/0000/example-o011> rdf:type knora-admin:DefaultObjectAccessPermission ;
     knora-admin:forProject <http://rdfh.ch/projects/0000> ;
     knora-admin:forResourceClass example:Biography ;
     knora-base:hasPermissions "CR knora-admin:ProjectAdmin|M knora-admin:ProjectMember"^^xsd:string .

<http://rdfh.ch/permissions/0000/example-o012> rdf:type knora-admin:DefaultObjectAccessPermission ;
     knora-admin:forProject <http://rdfh.ch/projects/0000> ;
     knora-admin:forProperty example:hasBiography ;
     knora-base:hasPermissions "CR knora-admin:ProjectAdmin|M knora-admin:ProjectMember|V knora-admin:KnownUser, knora-admin:KnownUser"^^xsd:string .

<http://rdfh.ch/permissions/0000/example-o013> rdf:type knora-admin:DefaultObjectAccessPermission ;
     knora-admin:forProject <http://rdfh.ch/projects/0000> ;
     knora-admin:forProperty example:hasBiographyValue ;
     knora-base:hasPermissions "CR knora-admin:ProjectAdmin|M knora-admin:ProjectMember|V knora-admin:KnownUser, knora-admin:KnownUser"^^xsd:string .

Would the user be allowed to see the link but prevented from accessing the resource itself?

benjamingeer · 2019-11-20T12:19:04Z

@mrivoal Could you please test this PR with your use cases and see if it does what you want?

Review should be based on testing the requested use case.

mrivoal · 2019-11-20T12:32:31Z

Ok. I will try and do this tomorrow, as it means having an up-to-date stack on my laptop and involve @loicjaouen or @gfoo's help.

mrivoal

I tested the use case I was mentioning. An unauthorized user is not allowed to see the restricted resource nor the link property that targets this resource. I think this is a consistent behavior.

If it is useful I can commit my use case, as it is based on Anything ontology.

benjamingeer · 2019-11-21T12:57:41Z

An unauthorized user is not allowed to see the restricted resource nor the link property that targets this resource. I think this is a consistent behavior.

Hmm, actually in our test, you can see the link value but not the target resource. But I've just now realised that I didn't push the test data for this before leaving Basel...

If it is useful I can commit my use case, as it is based on Anything ontology.

Yes please, that would be very helpful.

…urce class (RedThing)

mrivoal · 2019-11-21T13:27:08Z

An unauthorized user is not allowed to see the restricted resource nor the link property that targets this resource. I think this is a consistent behavior.

I meant in Salsah and Knora app. If there is a link between 2 resources, the label of the target resource is displayed in the resource which the origin of the link property. If the link was displayed, I guess the label of the target resource would be as well which might be giving too much information for a resource a user is not allowed to access.

What I see when I am logged in :

What I see as an UnkwownUser:

benjamingeer · 2019-11-21T13:31:27Z

I don't know what the SALSAH app does in this case, but the API returns the link value, containing only the IRI of the target resource. This has always been the case (in API v2) when requesting just one resource, and now it is (or should be) the case when you do a Gravsearch query.

gfoo · 2019-11-21T13:36:08Z

Done few tests and it seems to work.

Related to @mrivoal example, with my model is Person -hasNote-> Note, in my data test, the person is related to two notes, the second one is forbidden for the current user.

Now I can get the Person and not a ForbiddenRes (as required by our issue) and you provide as well the two link values in the result. The first one (not forbidden) with a "knora-api:linkValueHasTarget" and the second one (forbidden) with a "knora-api:linkValueHasTargetIri".

Can I consider that if you provide a "knora-api:linkValueHasTargetIri", that means this related resource is forbidden?

tobiasschweizer · 2019-11-21T13:47:54Z

please remember to adapt the Gravsearch design docs

benjamingeer · 2019-11-29T10:46:35Z

Can I consider that if you provide a "knora-api:linkValueHasTargetIri", that means this related resource is forbidden?

I guess so!

benjamingeer · 2019-11-29T12:19:35Z

As I was updating the docs for this PR, I've just realised that it produces inconsistent behaviour:

If the user does not have permission to see a matching main resource, it is replaced by a
proxy resource called knora-api:ForbiddenResource. If a user does not have permission
to see a matching dependent resource, only its IRI is returned.

What do you all think of this? Should we replace ForbiddenResource with something that just contains the IRI of the resource you don't have permission to see?

@tobiasschweizer @mrivoal @loicjaouen @gfoo

gfoo · 2019-11-29T13:39:12Z

I don't really understand why you want to return the IRI of the forbidden target? Is there a particular reason for that? Could we simply return a knora-api:ForbiddenResource in place of the knora-api:linkValueHasTargetIri?

The new specs will be:

If the user does not have permission to see a matching main resource or a dependent
resource, it is replaced by a proxy resource called knora-api:ForbiddenResource.

benjamingeer · 2019-11-29T14:34:14Z

I don't really understand why you want to return the IRI of the forbidden target? Is there a particular reason for that?

Because that's what the /v2/resources route does, and this PR was supposed to make Gravsearch do the same thing as the /v2/resources route.

We could return ForbiddenResource instead, but then we'd have to change /v2/resources to make them consistent. What do you think?

subotic · 2019-11-29T14:42:32Z

What does the API return if you request the returned (forbidden) IRI?

PS. Today is a university holiday and we don't work ;-)

benjamingeer · 2019-11-29T14:46:52Z

Today is a university holiday and we don't work ;-)

Sorry, I will stop working immediately. :)

gfoo · 2019-11-29T14:53:12Z

Today is a university holiday and we don't work ;-)

Sorry, I will stop working immediately. :)

Good idea, I have an old movie to re-watch :))) https://media.services.cinergy.ch/media/cinemanteaser174x240x2/ce00f79b734ebedb9fcb36dc65cf3fe0a30cf74a.jpg

gfoo · 2019-11-29T15:09:01Z

I don't really understand why you want to return the IRI of the forbidden target? Is there a particular reason for that?

Because that's what the /v2/resources route does, and this PR was supposed to make Gravsearch do the same thing as the /v2/resources route.

We could return ForbiddenResource instead, but then we'd have to change /v2/resources to make them consistent. What do you think?

I think that there is more consistency problems than I thought, why v2/resources returns that:

{
    "knora-api:error": "org.knora.webapi.NotFoundException: One or more requested resources were not found (maybe you do not have permission to see them, or they are marked as deleted): <http://rdfh.ch/0113/XiAMcfY2Skis3j1bd-QkmA>",
    "@context": {
        "knora-api": "http://api.knora.org/ontology/knora-api/v2#"
    }
}

instead of a ForbiddenResource. Not so sure that gravsearch should follow the rules of v2/resources in any cases.

Don't know what is the good solution, our first request was to not have a ForbiddenResource in place of main resource, it's done, great! To have or not the IRI of forbidden dependent resources is just an extra for my use case (I just need to know if this dependent resource is forbidden or not...)

gfoo · 2019-11-29T15:10:28Z

Can we have ForbiddenResource with v2/resources? or is it only for gravsearch?

benjamingeer · 2019-12-02T15:09:30Z

Now that we are saying that we don't try to pretend something doesn't exist if you don't have permission to see it, I don't think NotFoundException makes sense.

We could use HTTP 403 (Forbidden) with /v2/resources, but I think this wouldn't make sense if you requested two resources, and you only have permission to see one of them. Then you should still get that one, and you should also find out that you don't have permission to see the other one.

So I think we need something like ForbiddenResource both for /v2/resources and for Gravsearch, both for main resources and for dependent resources.

So we have two options: either ForbiddenResource or a LinkValue (or something similar) containing just the resource IRI.

One thing I don't like about ForbiddenResource is that it's rather big. I would prefer something more lightweight.

benjamingeer · 2019-12-02T15:20:44Z

I guess the easy solution right now, to make things consistent, would be to use ForbiddenResource.

gfoo · 2019-12-05T08:18:27Z

@benjamingeer Is there any pending questions here? are you waiting some feedbacks from us?

I cannot descently review your scala code (out of my skills) but about the specs it is fine for me and I refactored my code to follow what you implemented in this branch.

About your last remark:

I guess the easy solution right now, to make things consistent, would be to use ForbiddenResource.

If you are talking about v2/resources, it is an new issue, isn't?

benjamingeer · 2019-12-05T09:18:13Z

If you are talking about v2/resources, it is an new issue, isn't?

Yes, I guess we could do that in a separate PR. I would just need to know whether it would break existing client code. @tobiasschweizer

tobiasschweizer · 2019-12-05T09:29:44Z

I would just need to know whether it would break existing client code.

So instead of a non successful HTTP response the client would get a 200 with ForbiddenResource when requesting a resource from v2/resources with insufficient permissions, right?

benjamingeer · 2019-12-05T10:01:01Z

That’s what I was thinking, does it make sense to you? Do you know if client code would need to be changed?

tobiasschweizer · 2019-12-05T11:03:08Z

Do you know if client code would need to be changed?

I cannot think of any reason why this should be necessary. Instead of a failed HTTP request (non 200 HTTP response) a resource is returned like in all cases of successful responses. I would propose that the integration with the client lib be tested when doing the PR.

tobiasschweizer · 2019-12-05T11:06:03Z

in dasch-swiss/knora-api-js-lib, a unit test should be added for this case

fix(gravsearch): Unify permissions handling with full resource request.

523ad58

benjamingeer self-assigned this Nov 20, 2019

benjamingeer requested a review from mrivoal November 20, 2019 10:47

benjamingeer added API/V2 Gravsearch labels Nov 20, 2019

mrivoal previously approved these changes Nov 20, 2019

View reviewed changes

benjamingeer requested review from loicjaouen and gfoo November 20, 2019 12:20

benjamingeer mentioned this pull request Nov 20, 2019

Ben's PR history #571

Open

mrivoal approved these changes Nov 21, 2019

View reviewed changes

Add a use case for a link property targeting a restricted access reso…

213b493

…urce class (RedThing)

Benjamin Geer added 3 commits November 28, 2019 15:05

Merge branch 'develop' into fix/1344-gravsearch-permissions

ba957cb

test: Fix test data.

62b850a

test: Add test.

17af83e

Benjamin Geer added 2 commits November 29, 2019 13:13

test: Improve test, make it not interfere with other tests.

bc68fed

docs: Update docs.

d67cf36

test: Remove obsolete test class.

8f5b1fc

test: Fix test and test data.

1275d38

Merge branch 'develop' into fix/1344-gravsearch-permissions

3fd18c8

test: Fix test.

9eb0250

Merge branch 'develop' into fix/1344-gravsearch-permissions

ac90e82

benjamingeer added this to the 2019-12 milestone Dec 5, 2019

benjamingeer added the breaking/api label Dec 5, 2019

benjamingeer merged commit 064ca4a into develop Dec 5, 2019

benjamingeer deleted the fix/1344-gravsearch-permissions branch December 5, 2019 09:20

benjamingeer mentioned this pull request Dec 5, 2019

Use ForbiddenResource consistently #1543

Closed

subotic mentioned this pull request Dec 10, 2019

StillImageRepresentation uploaded by knora-xml-import cannot be displayed in Salsah 1.5 dasch-swiss/dsp-tools#21

Closed

This was referenced Jan 20, 2020

Change the way Gravsearch results are filtered according to permissions #1494

Closed

Filter Gravsearch results like the full resource request #1475

Closed

Gravsearch main query gets values that won't be returned #1464

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gravsearch): Unify permissions handling with full resource request #1521

fix(gravsearch): Unify permissions handling with full resource request #1521

benjamingeer commented Nov 20, 2019

mrivoal left a comment

benjamingeer commented Nov 20, 2019

mrivoal commented Nov 20, 2019

mrivoal left a comment

benjamingeer commented Nov 21, 2019

mrivoal commented Nov 21, 2019

benjamingeer commented Nov 21, 2019

gfoo commented Nov 21, 2019

tobiasschweizer commented Nov 21, 2019

benjamingeer commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

gfoo commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

subotic commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

gfoo commented Nov 29, 2019

gfoo commented Nov 29, 2019 •

edited

Loading

gfoo commented Nov 29, 2019 •

edited

Loading

benjamingeer commented Dec 2, 2019 •

edited

Loading

benjamingeer commented Dec 2, 2019

gfoo commented Dec 5, 2019 •

edited

Loading

benjamingeer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

benjamingeer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

fix(gravsearch): Unify permissions handling with full resource request #1521

fix(gravsearch): Unify permissions handling with full resource request #1521

Conversation

benjamingeer commented Nov 20, 2019

mrivoal left a comment

Choose a reason for hiding this comment

benjamingeer commented Nov 20, 2019

mrivoal commented Nov 20, 2019

mrivoal left a comment

Choose a reason for hiding this comment

benjamingeer commented Nov 21, 2019

mrivoal commented Nov 21, 2019

benjamingeer commented Nov 21, 2019

gfoo commented Nov 21, 2019

tobiasschweizer commented Nov 21, 2019

benjamingeer commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

gfoo commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

subotic commented Nov 29, 2019

benjamingeer commented Nov 29, 2019

gfoo commented Nov 29, 2019

gfoo commented Nov 29, 2019 • edited Loading

gfoo commented Nov 29, 2019 • edited Loading

benjamingeer commented Dec 2, 2019 • edited Loading

benjamingeer commented Dec 2, 2019

gfoo commented Dec 5, 2019 • edited Loading

benjamingeer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

benjamingeer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

tobiasschweizer commented Dec 5, 2019

gfoo commented Nov 29, 2019 •

edited

Loading

gfoo commented Nov 29, 2019 •

edited

Loading

benjamingeer commented Dec 2, 2019 •

edited

Loading

gfoo commented Dec 5, 2019 •

edited

Loading