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

[DS-4036] Delete EPersons even if they are referenced #2229

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@pnbecker
Copy link
Member

commented Oct 4, 2018

https://jira.duraspace.org/browse/DS-4036

DSpace references EPersons in different database tables like the
submitter of an item or like the EPerson that gets special rights
granted in the resourcepolicy table. This PR changes DSpace so it can
handle references that are set null instead of referencing an actual
EPerson. This is important to be able to delete EPersons which is
demanded by several data protection laws like GDPR in the European
Union.

@pnbecker pnbecker added this to the 7.0 milestone Oct 4, 2018

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

I mark this as "work in progress". Not that I currently do have to add anything specific, but I think we will vet this and maybe work on this together as port of DSpace 7.
At least commiters of DSpace should be able to change this PR.

@terrywbrady
Copy link
Contributor

left a comment

Very nice work. I have added a few questions and some minor suggestions to my review.

Show resolved Hide resolved ...i/src/main/java/org/dspace/authorize/dao/impl/ResourcePolicyDAOImpl.java
* @throws SQLException if there's a database problem
* @throws AuthorizeException if authorization error
*/
public void removeAllEPersonPolicies(Context c, EPerson e) throws SQLException, AuthorizeException;

This comment has been minimized.

Copy link
@terrywbrady

terrywbrady Oct 18, 2018

Contributor

What is the difference between removeAllEPersonPolicies() and deleteAllEPersonPolicies() (in ResourcePolicyDAO.java)? Should these names match?

This comment has been minimized.

Copy link
@pnbecker

pnbecker Nov 30, 2018

Author Member

@terrywbrady You're probably right. But we have this mismatch also between all of the other methods of the ResourcePolicyService and the ResourcePolicyDAO. In the DAO the term used is always "remove", in the Service it is "delete".

Show resolved Hide resolved dspace-api/src/main/java/org/dspace/content/WorkspaceItemServiceImpl.java
Show resolved Hide resolved dspace-api/src/main/java/org/dspace/content/dao/impl/ItemDAOImpl.java
Show resolved Hide resolved dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java
Show resolved Hide resolved .../org/dspace/xmlworkflow/state/actions/processingaction/ReviewAction.java
Show resolved Hide resolved ...e/xmlworkflow/state/actions/processingaction/SingleUserReviewAction.java
Show resolved Hide resolved dspace-api/src/main/resources/Messages.properties Outdated
Show resolved Hide resolved dspace-api/src/test/java/org/dspace/eperson/EPersonTest.java Outdated
Show resolved Hide resolved dspace-api/src/test/java/org/dspace/eperson/EPersonTest.java Outdated
@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

@terrywbrady @mwoodiupui Thank you for the review. Either I answered directly to your questions/suggestions or I changed the PR in the PR I just pushed. Can you please take a look if my answers/changes are satisfying?

[DS-4036] Delete EPersons even if they are referenced
DSpace references EPersons in different database tables like the
submitter of an item or like the EPerson that gets special rights
granted in the resourcepolicy table. This PR changes DSpace so it can
handle references that are set null instead of referencing an actual
EPerson. This is important to be able to delete EPersons which is
demanded by several data protection laws like GDPR in the European
Union.

@pnbecker pnbecker force-pushed the tuub:DS-4036 branch from e57a53f to 4529035 Nov 30, 2018

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I rebased the PR and resolved the conflicts.

@pnbecker pnbecker force-pushed the tuub:DS-4036 branch from 4529035 to 3890c11 Nov 30, 2018

@benbosman
Copy link
Member

left a comment

The behavior when the user is part of the workflow is not clear to me yet, it seems the item would move to different workflow state when deleting a user. This is something the repository admin may not expect to happen. I did also add a few inline comments specific to the workflow.
It's very hard to verify whether all use cases are covered where it's currently assumed an item has a submitter. So this implies if some parts are missing, a NullPointerException would still pop up.

If the user's confidential data (name, email address, …) would be replaced instead of deleting the EPerson, all these changes would no longer be required. That would avoid all the changes here, eliminates the risk of forgetting any customizations, and would ensure the items remain in their current state.
An alternative solution would be to link all the items to a single EPerson account, which will have the same advantages, but would require that account to be not deletable.

xmlWorkflowService.deleteClaimedTask(context, xmlWorkflowItem, pooledTask);

try {
workflowRequirementsService.removeClaimedUser(context, xmlWorkflowItem,

This comment has been minimized.

Copy link
@benbosman

benbosman Dec 19, 2018

Member

What happens if the user is in the pool, will they be reassigned?
What happens if the user is/was the only user in the pool?

taskListItemService.deleteByEPerson(context, ePerson);
} else if (StringUtils.equals(tableName, "cwf_pooltask") && isXmlFramework) {
PoolTaskService poolTaskService = XmlWorkflowServiceFactory.getInstance().getPoolTaskService();
poolTaskService.deleteByEperson(context, ePerson);

This comment has been minimized.

Copy link
@benbosman

benbosman Dec 19, 2018

Member

what happens if they're the only person in the pool

@pnbecker

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

Ben,

thank you for the review. I missed it in December and got aware of it by the end of last week. I know about the possibility to link "orphaned items" to a single user, often called ghost user. I agree, my approach is risking NPEs. But I'm doing this for the sake of a cleaner implementation in the long run. I want to avoid having to care about a ghost user everywhere, where we are dealing with EPersons.

You pointed out onw side-effects that we missed in deed. Thank you for finding that issue. A user being deleted may be the last user in a Workflow group. I think we have two bugs in DSpace that explains how I would expect DSpace to handle that case:

  1. If the group of users for a specific Workflow step is empty this step is skipped silently. If an administrator activates a workflow step, but leaves the group empty, this resolves in an unexpeted behavior: having a workflow step that is activated but skipped nevertheless. I think DSpace should keep the item in the workflow step, send a mail to the administrator and put a warning into the log files.

  2. If a user is added to a workflow step group, workflow item tasks are not assigned to that user. To make this concrete: if you have a workflow step with 500 items in the task pool and add a user to that workflow step, the user is still not seeing a single workflow item, until a new one is put into the pool. Then the user sees the new task only but not any of the tasks that existed before he/she was added to that workflow step's group.

If we would resolve both bugs, DSpace would not skip any steps anymore and administrators would have a chance to give people access to workflow tasks whose workflow groups are empty. This might also be a way to solve https://jira.duraspace.org/browse/DS-1941.

I know that we want to move to the XMLWorkflowSteps in DSpace 7. All my notes above relate to the default Workflows of DSpace 6 and I have to test if these issues are the same for the XMLWorkflows.

@benbosman

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@pnbecker

  • If the group of users for a specific Workflow step is empty, this step is skipped silently in the XML Workflow as well. So this will require a solution to avoid the workflow step from being skipped (e.g. refusing to delete an EPerson if it would create an empty workflow group, so the admin would need to solve that problem first)
  • If a user is added to a workflow step group, workflow item tasks which are in the pool are assigned to that user in the XML Workflow, so that problem should be solved because the Basic Workflow was dropped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.