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

Cookie preferences & End User Agreement per account - initial registries update #2933

Merged

Conversation

jonas-atmire
Copy link
Contributor

@jonas-atmire jonas-atmire commented Aug 19, 2020

References

Description

This adds in the initial metadatafields for the cookie preference related work.
The current fields have been provided by @artlowel as he is handling the Angular version implementation of this.

Instructions for Reviewers

This basically only adds in some metadatafields, and adds in a check to make sure that these metadatas exist and can be filled in on an eperson level.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

@artlowel artlowel linked an issue Aug 20, 2020 that may be closed by this pull request
@artlowel artlowel added this to Needs Reviewers Assigned in DSpace 7 Beta 4 Aug 20, 2020
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge. label Aug 20, 2020
@heathergreerklein heathergreerklein moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Aug 20, 2020
<dc-type>
<schema>dspace</schema>
<element>agreements</element>
<qualifier>end-user</qualifier>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have dspace.agreements.acceptedDate storing the timestamp of the acceptance. Anyway it would be better to add some scope note about how it will be used

Copy link
Member

@tdonohue tdonohue Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abollini : I think you might be misunderstanding the point of this metadata field. There's not just a single user/usage agreement, so the field dspace.agreements.acceptedDate seems wrong, as it implies there's one agreement with one date of acceptance.

Instead, there's (at least) two different agreements required by the GDPR Working Group:

  1. A cookie consent form, described in more detail here: Cookie Preferences per account dspace-angular#735 (comment)
  2. An End User Agreement, described in more detail here: User Agreement / Privacy Statement #2808 (comment)

So, this PR is creating two new metadata fields for the EPerson object:

  1. dspace.agreements.cookies to store an EPerson's answer to the Cookie consent form. My assumption is this field will store the list of cookies that the EPerson has chosen to allow (opt-in approach). It likely cannot be just a true/false field as we had talked previously about using something like https://klaro.kiprotect.com/#features
  2. dspace.agreements.end-user to store an EPerson's answer to the End User agreement. My assumption is that this field may be simply true or false based on whether they agree with the End User agreement. However, we could choose to have this field store the date/time of agreement.
    1. Arguably, if this field is ever false or null then that EPerson account should be blocked until they agree. It should not be possible to use DSpace if you don't agree to the End User Agreement.

Overall, I agree that we need to add in <scope_note> descriptions to both of these fields in order to describe the usage and valid/accepted values. Currently, while the fields look OK at a glance, I've had to make assumptions on how I expect them to be used (but I'm not sure whether my assumptions are correct).

@pnbecker : You may also want to review the direction of this small PR to provide any feedback on behalf of the GDPR Working Group

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. dspace.agreements.cookies to store an EPerson's answer to the Cookie consent form. My assumption is this field will store the list of cookies that the EPerson has chosen to allow (opt-in approach). It likely cannot be just a true/false field as we had talked previously about using something like https://klaro.kiprotect.com/#features

That is correct, it will store a copy of the klaro cookie, which is an array of cookieName/boolean pairs. The contents of this metadata field wil be set as the klaro cookie for the session every time you log in, and kept up to date every time you change something in the klaro panel.

  1. dspace.agreements.end-user to store an EPerson's answer to the End User agreement. My assumption is that this field may be simply true or false based on whether they agree with the End User agreement. However, we could choose to have this field store the date/time of agreement.
    i. Arguably, if this field is ever false or null then that EPerson account should be blocked until they agree. It should not be possible to use DSpace if you don't agree to the End User Agreement.

Correct. If this metadata field has any value other than true, the user will see the end-user agreement immediately after logging in, and can't do anything else until they agree. Disagreeing or canceling will log them out again. If there is an updated version of the user agreement the idea is that you delete this field for all EPersons, so they all have to agree again. We could store a timestamp here instead if that's preferred but I don't see a use case for it at the moment.

@tdonohue tdonohue changed the title Cookie preferences per account - initial registries update Cookie preferences & End User Agreement per account - initial registries update Aug 20, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonas-atmire and @artlowel : Overall, I approve of this PR. I just have very minor requests inline to better document the usage of these new fields. I'm OK with the fields as-is, and also OK with the end-user field simply having true/false values. If we discover a later need to store the date of acceptance, that can always be updated at a later time.

dspace/config/registries/dspace-types.xml Outdated Show resolved Hide resolved
dspace/config/registries/dspace-types.xml Outdated Show resolved Hide resolved
DSpace 7 Beta 4 automation moved this from Under Review to Reviewer Approved Aug 28, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me now. Thanks for the minor updates @jonas-atmire !

@pnbecker : Pinging you one last time on this PR to ensure you have a chance to give feedback (if necessary) prior to merger. I believe this aligns with the goals expressed by the GDPR working group, but would appreciate it if you could verify. It's a tiny PR, so it should only take you 5-10 mins to glance at it. (I plan to merge this on Monday. Once merged, we can always refactor later on of course.)

@tdonohue tdonohue merged commit bb6413a into DSpace:main Sep 3, 2020
DSpace 7 Beta 4 automation moved this from Reviewer Approved to Done Sep 3, 2020
@tdonohue tdonohue added this to the 7.0beta4 milestone Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

User Agreement / Privacy Statement Cookie Preferences per account
5 participants