-
Notifications
You must be signed in to change notification settings - Fork 753
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
Data Consent feature for DNN #2683
Conversation
…fo. Add UserAgreedToTerms and ResetTermsAgreement methods to set these fields.
… in portals where the DataConsent mechanism is active with Delayed Hard Delete of users
CREATE PROCEDURE {databaseOwner}[{objectQualifier}ResetTermsAgreement] | ||
@PortalId INT | ||
AS | ||
UPDATE {databaseOwner}{objectQualifier}UserPortals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it irritation/misleading, if HasAgreedToTermsOn has a value but HasAgreedToTerms not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sleupold I believe the reason to leave the value is that you can infer "last agreed" by this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IN this case, I would store "HasAgreedToTermsVersion" instead of "HasAgreedToTerms" and compare with PortalSetting "TermsVersion", which would be increased by the resetButton.
GO | ||
|
||
IF NOT EXISTS (SELECT * FROM {databaseOwner}{objectQualifier}EventLogConfig WHERE LogTypeKey = 'DATA_CONSENT_SUCCESS') | ||
BEGIN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of just keeping the past 10 records?
IMO success records should only be kept, if there is a concrete need - otherwise logging should turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Done. It's gone.
@PortalId INT, | ||
@UserId INT | ||
AS | ||
IF EXISTS (SELECT * FROM {databaseOwner}{objectQualifier}UserProfile up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this distinction is not correct, it doesn't cover superuser profiles properly.
You should distinguish upon @portalid parameter, whether to delete portal specific properties (positive value) or all properties (PortalID is Null or -1). It depends on Userportals records, whether to call with portalID (other userportals record with other portalID exists) or without (otherwise).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell what this is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great, and thank you for the contribution! It's going to make a lot of people quite happy to see this in the release notes for v9.4.0!
Can you easily move the sql changes into the main 940 file? This early in the cycle I'd rather keep them in a single file, but since we really don't have anything defined for sql changes it's really just my personal preference for release management and upgrade performance and not a required change at all.
/AzurePipelines run "DNN [Publish Artifacts]" Result: CI task didn't set the correct version, so I cancelled the build. Will trigger it again shortly after another test build runs. |
…zation as it is too risky for now.
/AzurePipelines run "DNN [Publish Artifacts]" |
Summary
This PR implements a number of enhancements to the platform to allow administrators to require users' consent to storage and usage of data. This is an essential part of GDPR compliance in Europe.
In essence the login experience is enhanced with a new step "DataConsent" which kicks in once the admin has activated the feature for the site. A user's consent is recorded as well as the time it was done. The action is also logged. This allows basic forensics to take place. For more advanced scenarios this solution also allows the Data Consent step to be done elsewhere on the site through a "Consent Redirect" setting.
Account removal
An integral part of consent is the option to opt out and even delete one's profile. Because it is hard to tell if all extensions will respond well to a user being eliminated from the database, the solution provides three mechanisms for user account deletion (by the user):
Note that to achieve number 3, a scheduled task has been added.
Discussion
Although care has been taken to provide a powerful mechanism that will address most needs for GDPR compliance in this area it is noted that always be more powerful and complete. The criteria with which I began this were that it would be as minimal an intervention in terms of code changes and additions and it would allow 3rd party developers to leverage this to create a more powerful system.
Testing
Note what needs to be done is to check more complex integration scenarios With that I’m thinking: