Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support adding creator to other entity types as well #122

Closed
nichtich opened this issue Oct 5, 2020 · 11 comments
Closed

Support adding creator to other entity types as well #122

nichtich opened this issue Oct 5, 2020 · 11 comments
Labels
feature Additional functionality
Milestone

Comments

@nichtich
Copy link
Member

nichtich commented Oct 5, 2020

Annotations are saved with their creator automatically set via user. For bartoc.org we also want creators to be set on import via PUT/POST. This could be made generic by setting a config value withUser to the entity type.

@nichtich nichtich added the feature Additional functionality label Oct 5, 2020
@nichtich nichtich added this to the 1.2.0 milestone Oct 5, 2020
@stefandesu
Copy link
Member

We need to specify exactly what should happen in which case.

  • Adding a new entity should add the current user as creator. (Should existing creators be moved to contributor?)
  • Updating an entity should add the current user as contributor if they are not themselves creator or already in contributor. (???)

@nichtich Can you explain what you mean with the withUser thing, and maybe expand on my start here? Thanks!

@nichtich
Copy link
Member Author

nichtich commented Nov 5, 2020

Let's ignore bulk import via admin and anonymous writing and focus on cases with auth: true.

  • The payload fields creator and contributor will alway be ignored
  • Adding a new entity should add the current user as creator
  • Updating an entity should add the current user as contributor unless they are in creator or contributor

Open question:

  • which identity URI of the user should be used? (I think this is what withUser was about)
    a) configured in jskos-server (e.g. field identity: ["orcid", "wikidata"] to try orcid URI first, Wikidata as second fallback)
    b) selected by the client (I think this is done in Cocoda but how to tell jskos-server which identity to use?)
  • only the user URI or additional information such as name? (I think URI only)

@stefandesu
Copy link
Member

Okay, this definitely still needs a lot of discussion.

Let's ignore bulk import via admin and anonymous writing and focus on cases with auth: true.

  • The payload fields creator and contributor will alway be ignored
  • Adding a new entity should add the current user as creator
  • Updating an entity should add the current user as contributor unless they are in creator or contributor

👍

  • which identity URI of the user should be used?

Currently, this is not consistent for entity types.

  • For mappings, the creator field is not touched in any way, i.e. someone could write mappings with someone else's identity (but they wouldn't be able to edit or delete it anymore).
  • For annotations, the creator field is set to { id: user.uri, name: user.name }, i.e. the Login Server URI and name. (I can't remember the reason why we did this, but having consistent user URIs here makes filtering by creator easier.)

We need to find a solution that can be applied consistently (and doesn't break existing applications).

  • only the user URI or additional information such as name? (I think URI only)

If we go for URI only, we couldn't show any names in Cocoda for mappings and annotations anymore.

I agree that creator and contributor in the payload should be ignored. (Although you could argue that allowing arbitrary values would be good, either if you want to make it clear that this is not a mapping you have created yourself, but taken from somewhere else, or if you want to give someone else access rights.)

However, I think the user should be able to choose which identity and data is added to a mapping. My suggestion would be:

  • Use the Login Server URI by default (which makes the mapping anonymous for anyone except for those with access to the user database).
  • Have a parameter like you suggested, for example withIdentity which is the key for an identity provider (orcid, wikidata, github, etc.) to override the default.
  • Include the name by default (either the identity name or the Login Server name), but add another parameter (withName=false?) that can disable this.

What do you think?

@nichtich
Copy link
Member Author

nichtich commented Nov 5, 2020

If we go for URI only, we couldn't show any names in Cocoda for mappings and annotations anymore.

Ok, then add a name

if you want to make it clear that this is not a mapping you have created yourself, but taken from somewhere else, or if you want to give someone else access rights.)

Adding someone else as contributor is a relevant use case (especially for concordances), so one should also be allowed to write this field but this is annother issue to be solved later when required.

  • Use the Login Server URI by default

👍

  • Have a parameter like you suggested, for example withIdentity which is the key for an identity provider (orcid, wikidata, github, etc.) to override the default.

As query parameter better use the full identity URI, e.g. identity=https://github.com/nichtich

  • Include the name by default (either the identity name or the Login Server name), but add another parameter

Use identityName as optional parameter.

This should also allow clients to change your existing identity URI and/or name by saving the same item without modification but different identity/identityName

@stefandesu
Copy link
Member

Got it. 👌 Last question: If the full identity URI is not actually available in the user's identities, should we:

  • Fall back to the Login Server URI?
  • Throw an error?
  • Write it anyway?

@stefandesu
Copy link
Member

stefandesu commented Nov 6, 2020

I would suggest:

  • If crossUser is set to true for the current action (this would mean allowing that property for create), then any identity URI can be used. (Question: is it allowed to provide no creator at all? I think this should be allowed if auth: false is given.)
  • If crossUser is not set or false, only a valid identity of the authenticated user can be used (otherwise throw a ForbiddenAccessError).

I know you didn't want to consider anonymous writing and auth: false, but I think we need to because we might otherwise break existing functionality. Since anonymous writing implies crossUser: true, this case is set above. But what about auth: false together with crossUser: false? Is that even a valid configuration? If we don't require authentication, we can't compare the user with the creator anyway.

@stefandesu
Copy link
Member

stefandesu commented Nov 6, 2020

I updated json-anystream to give us the possibility to adjust objects before they are emitted to the stream, so we can do this at a central point (middleware) for all entities equally.

The following is a suggestion for implementing this in jskos-server.

Note that the user getting to this point in code means that they have the rights to perform the current action (this is separate from checking the creator for PUT/PATCH/DELETE though).

For all: Existing fields creator and contributor on the object to be adjusted will be removed.

POST

  • Assemble a creator object for the current request
    • Use req.user together with the params identity and identityName
    • If identity is not given or empty, use the Login Server URI
    • If identityName is given, but empty, no name should be saved in creator
  • Set the creator
    • Note that this has to be handled separately for annotations because they use an object instead of array and id instead of uri

PUT/PATCH

  • Get existing entity from database
    • Throw 404 if it doesn't exist
  • Assemble creator object (see POST above)
  • Check if user is allowed to update the entity
    • Throw 403 if not
  • Check if the user is already in creator or contributor
    • if yes: update with creator object if necessary (to update identity/name) (if they are in contributor, move them to end of list)
    • if not: push creator object to contributor (also set creator if there is no other creator)

Edit: Updated with how this is actually handled.

@nichtich
Copy link
Member Author

nichtich commented Nov 6, 2020

All right, except for creator and contributor fields should be ignored in the payload.

question: if they are in contributor, should we move them to the top of the list to implicate that they last edited the element?

No, the order is not relevant.

DELETE: Only mentioning this because I have a question: Should contributors be able to delete an entity? I would say no.

No, but not part of this issue anyway.

@stefandesu
Copy link
Member

I started an implementation and it seems to work well, but there is still more do be done. Implementations for all services (except annotations which I have done already) need to be adjusted, and we should add some tests to ensure correct implementation.

@stefandesu
Copy link
Member

stefandesu commented Nov 13, 2020

Still missing:

  • Tests to ensure correct implementation
  • Add note about this in README

@stefandesu
Copy link
Member

Merged into dev!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Additional functionality
Projects
None yet
Development

No branches or pull requests

2 participants