gbv / jskos-server

Web service to access JSKOS data
https://coli-conc.gbv.de/api/
MIT License
6 stars 4 forks source link

Reassess `handleCreatorForObject` method #153

Closed stefandesu closed 2 years ago

stefandesu commented 2 years ago

Currently, jskos-server overwrites the creator and contributor fields of all incoming entities with its own logic. There are several issues with that, however:

Maybe, instead of completely overwriting those two fields, the method should check if certain expected things are given (like that the identity performing the request is part of either creator or contributor, and that it should definitely be in creator for POST requests, for example), but keep the changes that were performed.

What do you think, @nichtich?

stefandesu commented 2 years ago

Also don't forget to adjust the tests if this functionality is changed. 🙈

nichtich commented 2 years ago

Currently, jskos-server overwrites the creator and contributor fields of all incoming entities with its own logic.

I thought it only adds values to creator or contributor but this is not fully clear from the application of handleCreatorForObject in bodyParser. In any case we have three sources of creator/contributor data to be merged:

Furthermore the logic required for #99 for editing concordances and the logic for editing concept schemes is not the same. I need to think about the other entity types first but just adding another if-branch for concordances may be easier.

stefandesu commented 2 years ago

My question is if it's possible to simplify and unify this logic. Do we even need to adjust the fields or can we trust the client that what they are sending is intended? Of course there need to be a few hardcoded rules, like that there needs to be at least one creator, and if there's no creator field on a POST request, that the authenticated account is added there. But other than that, I think it should be fine just removing the rest of the logic. Maybe I'm missing something though.

nichtich commented 2 years ago

I'll try to summarize editing editing and how it should affect creator and contributor:

Creation of entities:

Thus a newly created entity can have arbitrary contributor and

Update of entities:

Thus modification of entities

nichtich commented 2 years ago

Note that we have two use cases for crossuser editing by authenticated users:

The logic described in my previous comment is not about whether an action is allowed but how to set fields creator/contributor, if the action is allowed, so I think both use cases are possible by different combination of configuration of identities and crossuers (?) Originally we introduced crossuers to allow editing everything by any user, do we still need this?

stefandesu commented 2 years ago

I like that we are narrowing in on a solution that works with the existing system. There's just one thing that I can't fit into your solution: How do we deal with editing mappings that belong to a concordance? This needs to be handled as an exception in any case because we always need to check the creator/contributor of the concordance. I'm also not sure whether it makes sense to override the creator if an entity is updated (and the creator override is not part of the updated payload). Although, this only happens for crossUser==false and it that case, in theory, creator should already include the authenticated account.

One more thing: With your solution, a contributor of a concordance is able to change concordance metadata as well, right? This is in conflict to what we discussed in #99, unfortunately.

nichtich commented 2 years ago

One more thing: With your solution, a contributor of a concordance is able to change concordance metadata as well, right? This is in conflict to what we discussed in #99, unfortunately.

No, this issue is only about how the creator and contributor field can be modified. Checking whether an account can modify a record at all, is on another level. I'll try to summarize the other questions in another post

stefandesu commented 2 years ago

I started implementation in branch issue-153. The tests haven't been adjusted yet.

stefandesu commented 2 years ago

I have now implemented the changes, but with a few modifications that I thought made more sense:

My reasoning for this was that the creator of an entity shouldn't actually change, even if it was updated by another user, because that other user is not the creator. However, I concur that it might be necessary to allow changes in creator, for example to give another user rights to edit a particular entity. Right now, I don't think we have a use case for this though.