Mark resources and values as deleted, and check permissions on links

[benjamingeer]

The code for marking resources and values as deleted was incomplete and incorrect, so this is a reimplementation.

Previously, we had to make a new version of a value to mark it as deleted. Now this is necessary only for LinkValue (because its reference count has to be decremented to 0 first).

All resources and values that are marked as deleted now have a deleteDate as well as isDeleted true.

I implemented permission-checking on link values in the resources responder and the search responder.

Permissions are now stored in a single string literal as per #196, using the datatype property knora-base:hasPermissions. The format of this literal is documented in knora-base.tex.

I've also refactored some things in the resources responder, the values responder, and the routes and tests, and added comments.

Closes #53: When a TextValue containing standoff links is deleted, decrement the links' reference counts.

Closes #88: What permissions should a LinkValue for a standoff link have?

Closes #89: Permissions on links are not checked correctly.

Closes #132: Delete a resource.

Closes #182: Handle Permissions on Properties correctly when searching.

Closes #196: Store permissions in a single triple for more efficient queries.

[tobiasschweizer]

Great, you fixed the bug that on value deletion some strange value IRI was written back in the response. Now it is the same as the one that was sent in the original request.

Hpwever, I do not fully understand the documention for the parameter id for DeleteValueResponseV1:

• @param id the IRI of the value that was marked as deleted. This may be the same value IRI that was given in

  •             the [[DeleteValueRequestV1]], or it may be the IRI of a new version of that value.
    

How can there be a new version of that value be returned when it has been deleted just now? Concurrency?

[tobiasschweizer]

I have just found a bug in ExtendedSearch: We forgot to the statement:

?resource knora-base:isDeleted false .

[tobiasschweizer]

I added a fix to correctly URL encode IRIs for the delete request

[benjamingeer]

How can there be a new version of that value be returned when it has been deleted just now?

This is explained in the design documentation:

https://github.com/dhlab-basel/Knora/blob/wip/delete-values/docs/rst/knora-api-server/design-documentation/triplestore-updates.rst

Normally, a value is marked as deleted without creating a new version of it. However, link values must be treated as a special case. Before a LinkValue can be marked as deleted, its reference count must be decremented to 0. Therefore, a new version of the LinkValue is made, with a reference count of 0, and it is this new version that is marked as deleted.

[benjamingeer]

you fixed the bug that on value deletion some strange value IRI was written back in the response. Now it is the same as the one that was sent in the original request.

This wasn't a bug. Please read the description of this pull request above, paying special attention to this sentence:

Previously, we had to make a new version of a value to mark it as deleted. Now this is necessary only for LinkValue (because its reference count has to be decremented to 0 first).

[benjamingeer]

The old behaviour was also documented:

https://github.com/dhlab-basel/Knora/blob/develop/docs/rst/knora-api-server/design-documentation/triplestore-updates.rst

'Deleting' a value means creating a new version, marked with knora-base:isDeleted and pointing to the previous version.

[tobiasschweizer]

Ok, now I understand why in the case of a link a new version has to be created.

Could you please add a sentence to the documentation for case class DeleteValueResponseV1 referring to the documentation you have mentioned?

[tobiasschweizer]

Did you remove the constraint because this can apply to a resource and/or a value?

[benjamingeer]

Yes.

[tobiasschweizer]

I do have a problem of understanding regarding the deletion of LinkValues. The following code ist from ValuesResponderV1.scala:

case linkValue: LinkValueV1 =>
                    // It's a LinkValue. Make a new version of it with a reference count of 0, and mark the new
                    // version as deleted.

                    val linkPropertyIri = knoraIriUtil.linkValuePropertyIri2LinkPropertyIri(findResourceWithValueResult.propertyIri)

                    for {
                        sparqlTemplateLinkUpdate <- decrementLinkValue(
                            sourceResourceIri = findResourceWithValueResult.resourceIri,
                            linkPropertyIri = linkPropertyIri,
                            removedTargetResourceIri = linkValue.objectIri,
                            userProfile = deleteValueRequest.userProfile
                        )

                        sparqlUpdate = queries.sparql.v1.txt.deleteLink(
                            dataNamedGraph = settings.projectNamedGraphs(findResourceWithValueResult.projectIri).data,
                            triplestore = settings.triplestoreType,
                            linkSourceIri = findResourceWithValueResult.resourceIri,
                            linkUpdate = sparqlTemplateLinkUpdate,
                            maybeComment = deleteValueRequest.deleteComment
                        ).toString()

                    } yield (sparqlUpdate, sparqlTemplateLinkUpdate.newLinkValueIri)

What happens if the direct link has a reference count of 2? Then it only should be decremented to 1, but the direct link should not be deleted, right?

This is how I understand this line: val deleteDirectLink = linkValueQueryResult.directLinkExists && newReferenceCount == 0

But in the template deleteLink, there is the following check taking place:

@* Delete the direct link. *@
        @if(linkUpdate.deleteDirectLink) {
            ?linkSource ?linkProperty ?linkTarget .
        } else {
            @{throw SparqlGenerationException(s"linkUpdate.deleteDirectLink must be true in this SPARQL template"); ()}
        }

So in our example that would be false since the reference count would be 1.

[benjamingeer]

What happens if the direct link has a reference count of 2?

This, too, is explained in triplestore-updates.rst:

For consistency, every LinkValue contains a reference count. If the link property
is not knora-base:hasStandoffLinkTo, the reference count will always be either 1
(if the link exists) or 0 (if it has been deleted, in which case the link value will also be
marked with knora-base:isDeleted).

[benjamingeer]

See also the documentation in knora-base.pdf:

valueHasRefCount (1) The reference count of the link. This is meaningful when the LinkValue describes resource references in Standoff text markup (see Section 4.1.3). Otherwise, the reference count will always be 1 (if the link exists) or 0 (if it has been deleted).

[tobiasschweizer]

ok, so only a TextValue's standofflinks could create a ref count > 1.

in decrementLinkValue, I found the following comment:

// There's already a LinkValue for links between these two resources. Decrement its
                    // reference count.

Isn't this misleading? Of course there is "already a link". It is the one that you attempt to delete.

[benjamingeer]

That's in a match-case expression that looks at the result of a SPARQL query to get the details of the link. The link should exist, but if it doesn't, the query result will be None. Look at the comment under case None.

[tobiasschweizer]

Wouldn't it be better to say that you found what you expected in the first case?

[tobiasschweizer]

Ok, I see now what decrementLinkValue does in case of a standoff link: the direct link will only be deleted if the ref count reached 0.

[benjamingeer]

I'm going to keep working on #182. It also just occurred to me that the resource context query doesn't check permissions on isPartOf links, either.

[tobiasschweizer]

Ok, I will try to finish the review of this pull request by tomorrow.

If you have time could you have a look at #101 (comment)? Thx.

[benjamingeer]

No need to review this again yet, because #182 is going to take a lot more work.

[benjamingeer]

The main things that have changed since this pull request was last reviewed:

• The resources responder checks permissions on link values for incoming and outgoing links in response to ResourceFullGetRequestV1, as described in What permissions should a LinkValue for a standoff link have? #88 (comment).
• Link values for standoff links now always belong to knora-base:SystemUser.
• I redesigned full-text and extended search to check permissions on values (including link values), as described in Handle Permissions on Properties correctly when searching #182 (comment).
• knora-base:valueHasString is required on knora-base:Value again, because it's needed in search results.
• I refactored some things in the resources responder, values responder, routes, and tests, for clarity and consistency, and added comments.
• I added tests in ValuesResponderV1Spec, ResourcesResponderV1Spec, and SearchResponderV1Spec.
• I updated knora-base.txt and the RST docs.
• I reformatted all the code.

The resources responder still doesn't check permissions on link values for ResourceContextGetRequestV1. I tried implementing that, but it was much too slow (the query took over a minute). I think for now we can live without this check. Since the context query is actually just a specialised search, and since I don't think it can be implemented efficiently anyway, I think we should aim to eliminate it in API v2.

So I think this pull request is ready for review again.

[benjamingeer]

I've implemented #196: the permissions of each resource or value are now stored in a single triple, using the datatype property knora-base:hasPermissions. The format of the literal object of this property is documented in knora-base.tex.

[benjamingeer]

Since permission checking is now much faster, I added permission checking on link values for ResourceContextGetRequestV1.

[tobiasschweizer]

I have a question regarding permission checking of incoming links. In ResourcesResponderV1 when incoming links are queried, it is checked if the user has the permission to see the referring resource and if he has permissions to see the link (by looking at the reification).

However, in case of a standoff link, the owner is the SystemUser (hasStandoffLinkToValue). So what would happen in that case? Would that case have to treated specially here?

When querying links (whether outgoing or incoming), ensure that the user has permission to see the source resource and the target resource. Also ensure that the user has permission to see the LinkValue, unless the link property is hasStandoffLinkTo. We're justified in making this property a special case because it's system-maintained as a side effect of standoff.

If I get this right, standoff links should always be displayed. However, the user might not be able to see the underlying TextValue that gives the link semantics.

[tobiasschweizer]

The new way of representing permissions required the test data to be changed. I will have to do that for the BEOL test server. What would be the easiest way to do that? I can make a dump, change the data and load it back in.

[benjamingeer]

However, in case of a standoff link, the owner is the SystemUser (hasStandoffLinkToValue). So what would happen in that case? Would that case have to treated specially here?

Yes, it's treated specially. To see a standoff link, the user only needs permission to see the source and target resources. This is documented in knora-base.tex section 5.2 (Permissions):

"To view a link between resources, a user needs permission to view the source and target resources. He also needs permission to view the LinkValue representing the link, unless the link property is hasStandoffLinkTo."

And in section 4.1.3 (StandoffLink):

"Link values created automatically for resource references in standoff are automatically visible to all users, as long as they have permission to see the source and target resources."

This special case is implemented in PermissionUtilV1.getUserPermissionOnLinkValueV1.

The new way of representing permissions required the test data to be changed.

I wrote a program to change the test data. The test data on the wip/delete-values branch has already been changed. To update the permissions in any Turtle file, you can use this command in SBT:

org.knora.webapi.util.TransformTestData permissions input-file output-file

The first argument currently has to be permissions, but this is intended to be a general-purpose framework for transforming test data in different ways.

[tobiasschweizer]

got it: if (predicateIri == OntologyConstants.KnoraBase.HasStandoffLinkTo) { Some(permissionsToV1PermissionCodes(OntologyConstants.KnoraBase.ViewPermission)) }

Regrading permissions: Great, I will try to run the program with he beol data

[tobiasschweizer]

You refactored search so only one query is done (before, we had separate queries for paging and the actual results).

Now, search results may be filtered out if the user has no permissions to see them (changing the number of search results).

Does this get us into trouble somehow? I do not remember the reason, but we designed the query so we could do the counting directly on the basis of the results returned by the triplestore.

[benjamingeer]

The previous design relied on the triplestore to do counting and paging. The SPARQL had to count and return one row per matching resource so the OFFSET and LIMIT would be correct for paging. The revised design does all the counting and paging in Scala, so it's fine if the SPARQL returns more than one row per matching resource.