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

Add Privacy System to smf #4892

Closed
wants to merge 34 commits into from
Closed

Conversation

@albertlast
Copy link
Collaborator

@albertlast albertlast commented Aug 4, 2018

Since it look like that some basic function should be included: https://www.simplemachines.org/community/index.php?topic=560206.0
I start with the topic and look how far i come.

  • Admin UI-Setting
  • User Download Section
  • Admin Download Secttion
  • Policy Management:
  • Create/Update
  • Delete
  • Evaluate which user is aggreed to which version
  • Enforce old aggrement to update to new one
  • Display the approved and new policy to the user
  • Integration into registration

Defined scope of this pr to deliever the core function of the core function,
not in scope is to provide finall looking or texting version.
For this is the topic to big in my eyes to do this all by my self.

Needs the fetch_all changes from pr: #5048

albertlast added 3 commits Aug 4, 2018
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Aug 5, 2018

While working on this,
came the question up,
which field should be included for profile data,
messages and personal messages.

To give you an idea of the status,
personal data:
privacySMFprofile-9.zip
messages:
privacySMFmessages-3.zip
personal messages:
privacySMFpmessages-1.zip

Signed-off-by: albertlast albertlast@hotmail.de
@frandominguez03
Copy link
Member

@frandominguez03 frandominguez03 commented Aug 5, 2018

https://www.simplemachines.org/community/index.php?topic=560645.0

  • Data export, so users may (if you allow them to) export their profile data. (Profile including IP address(-history), posts (optional), personal messages (optional)). In the future, SMF might get an option to restore the basic profile of another site.
@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Aug 5, 2018

The url is not visable by contributer.

@wintstar
Copy link

@wintstar wintstar commented Aug 5, 2018

The url is not visable by contributer. https://www.simplemachines.org/community/index.php?topic=560206.0

Yes, i can not read this topic when i logged in.

The topic or board you are looking for appears to be either missing or off limits to you.

@illori
Copy link
Contributor

@illori illori commented Aug 5, 2018

that is a topic in a semi private topic @frandominguez03 please only link to public topics on the forum.

@frandominguez03
Copy link
Member

@frandominguez03 frandominguez03 commented Aug 5, 2018

@illori Isn't the topic on SMF Team Blog board?

@frandominguez03
Copy link
Member

@frandominguez03 frandominguez03 commented Aug 5, 2018

That's the one I linked in my message.

@illori
Copy link
Contributor

@illori illori commented Aug 5, 2018

oh sorry it was @albertlast that linked to the private topic.

@frandominguez03
Copy link
Member

@frandominguez03 frandominguez03 commented Aug 5, 2018

Anyway he's saying that he doesn't have access to the topic I linked, which means he doesn't have access to that board. Can you have a look at it when you have a spot?

@illori
Copy link
Contributor

@illori illori commented Aug 5, 2018

should be fixed now.

@wintstar
Copy link

@wintstar wintstar commented Aug 5, 2018

should be fixed now.

No, i can not read topic=560206.0, linked from alberlast.

@frandominguez03
Copy link
Member

@frandominguez03 frandominguez03 commented Aug 5, 2018

No, you're not supposed to read that. There was an error in which the one I linked couldn't be viewed by albertlast because he belongs to a special group.

@illori
Copy link
Contributor

@illori illori commented Aug 5, 2018

that is in a private board, it should not have been linked here in the first place.

@wintstar
Copy link

@wintstar wintstar commented Aug 5, 2018

that is in a private board, it should not have been linked here in the first place.

Then a participation makes no sense.

albertlast added 8 commits Aug 6, 2018
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
@@ -101,6 +101,22 @@
// Like settings.
$txt['enable_likes'] = 'Enable likes';

// Privacy settings.
$txt['enable_privacy_userexport'] = 'Enable Userdata Export by the User';
$txt['privacy_userexport_own'] = 'Allow to Download ther own Data';

This comment has been minimized.

@frandominguez03

frandominguez03 Aug 7, 2018
Member

ther -> their

This comment has been minimized.

@albertlast

albertlast Aug 7, 2018
Author Collaborator

Your comment remind me,
to define the scope of this pr.

I will change this one thing,
but please look over the complet text after the pr is merge,
so you can check if the text make sense in the context where it's used.

This comment has been minimized.

@frandominguez03

frandominguez03 Aug 7, 2018
Member

Sure, don't worry. I'll take a look at all the text strings once you're finished.

albertlast added 2 commits Aug 7, 2018
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
@wintstar
Copy link

@wintstar wintstar commented Aug 15, 2018

Yes, it works now. Browser cache was the cause.

@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Sep 16, 2018

@Sesquipedalian any status about this here?

@Sesquipedalian Sesquipedalian added this to the RC2 milestone Sep 18, 2018
@Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Sep 18, 2018

There's just a lot to test in this PR and my free time this autumn has been more limited than I had expected, that's all. I've added this to the RC2 milestone so that we can be sure to include it in the final release without having it hold up the release of RC1 in the meantime.

Copy link
Member

@Sesquipedalian Sesquipedalian left a comment

I haven't yet been able to fully review this yet, so I may need to request more changes later, but here is one request:

SMF already has a system for exporting user profile information that we can and should use. Just go to index.php?action=.xml;sa=profile;u=20;type=smf and it will output the profile info for user 20 (if you have permission to view it, of course). So instead of these new functions exporting user profile data, all we need is to add a link in the profile template pointing to that URL. Since this method already exists and since XML data is easier to manipulate than CSV data if necessary later, it'd be better to use this approach.

So, please change this PR to direct the user to that address in your template_getProfileData() function, and then delete the functions to generate the CSV file.

@Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Oct 15, 2018

As a followup to my review comment above, I see that your getProfileData() function can export profile info, posts, and private messages. The existing XML export method doesn't currently have a way to export posts or private messages, but I can create one easily enough. In fact, I am already going to be creating that soon for SMF 2.0, so it will be easy enough to port that over to 2.1.

@wintstar
Copy link

@wintstar wintstar commented Oct 15, 2018

I see only one problem. It is not multilingual.

profile-xml

@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Oct 15, 2018

Well i looked short in the function (News.php ShowXMLFeed) and it takes my pov the wrong route,
first issue that it use xml, xml is a dead format.

It's hard to read when you output many data,
it's got a huge overheader,
the importing of xml into excel(similiar tools) or in a database is very hard.
This format doesn't fit in here.

Csv is the right way of doing.

The second issue is the authority check is wrong for this use case (i need to be able to define if they allowed to download ther own data and or of some others).

it use the {query_see_board} auth check which is wrong to do this here.

The code look like run very slow not good to export hugh amount data,
easy to ddos this function.

So in my recommandtion drop the old stuff or don't use this for this use case here.

@Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Oct 22, 2018

XML is not at all a dead format. It may have a higher overhead, but it is also better at communicating information reliably between systems. CSV is a fragile format where meaning is dependent on the order of the columns. If the output from one forum includes columns that another forum doesn't expect, or has in a different order, or anything like that, using CSV as an import/export format is going to be messy and possibly break things.

Still, I'd say that there are uses for both formats, and we can and should offer both export formats. Then the user can choose the format that best meets her needs.

@wintstar
Copy link

@wintstar wintstar commented Oct 23, 2018

One could also simply create a page where the user can print his data. Printing not only in paper form but also as PDF is possible with most operating systems.

$profileData[1] = $profile;
call_integration_hook('integrate_getProfile_profile', array(&$profileData));
}
elseif ($mode == 'messages') // messages

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 23, 2018
Member

A user could have thousands of posts, and if he does, processing all of them at once like this will overwhelm the server—either it will run out of memory, or it will run out of time, or both. So, this needs some sort of method to work through them in manageable batches. Writing to a temporary file and automatically reloading the page every few seconds, like I did with the XML version, would probably work well here.

This comment has been minimized.

@albertlast

albertlast Oct 28, 2018
Author Collaborator

100k messages takes 500 ms -> looks good to me
the file stuff got many weakness
it created by his self a new gdpr topic
it could be infected by evil software
it take more time to proceed -> the cpu load is overall higher

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 29, 2018
Member

How much memory was used to process those 100,000 messages all at once?

This comment has been minimized.

@liroyvh

liroyvh Nov 1, 2018
Member

@Sesquipedalian That's very important indeed, especially problematic on more aggressively resource-limited shared hosting. Plus hosting many boards on a server executing that kind of queries can become a nuisance. Always assume low resources is probably a good idea.

This comment has been minimized.

@albertlast

albertlast Nov 1, 2018
Author Collaborator

It needs to be realistic,
100k message is more than sm.org got
And you will not host setup like this on freehost

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Nov 1, 2018
Member

That's not an answer to the question, @albertlast. 🤨

There are very good reasons why SMF uses paging to perform any operation involving large numbers of table rows. Those reasons apply here just as much as they do anywhere else.

This comment has been minimized.

@albertlast

albertlast Nov 1, 2018
Author Collaborator

you didn't awnser the question for the xml solution.

I test some solution,
handle the size better.

Sources/Profile-Actions.php Show resolved Hide resolved
Sources/Profile-Actions.php Show resolved Hide resolved
elseif ($mode == 'pmessages')
{
$request = $smcFunc['db_query']('','
SELECT pm.msgtime, pm.subject, pm.body

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 23, 2018
Member

This query grabs PMs from the user and PMs to the user, but the exported data doesn't indicate who was the author and who were the recipients. It would be more helpful if each row identified both.

This comment has been minimized.

@albertlast

albertlast Oct 28, 2018
Author Collaborator

This is indended,
because otherwise you export userdata from a different user in your data and
never allowed you to download his data.

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 29, 2018
Member

The displayed names of other users don't count as private data, so it is perfectly fine to include them in the downloaded information.

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 29, 2018
Member

Just to clarify, I'm asking for pm.from_name and GROUP_CONCAT(COALESCE(mem.real_name, mem.member_name)) AS to_names to be added to the SELECT in this query (which will require joining the members table, of course), and for those values to be included in the output columns.

This comment has been minimized.

@albertlast

albertlast Oct 29, 2018
Author Collaborator

In my eyes you don't need this,
would be enough to mark if you send or receive the message and maybe the technical user_id from the other.

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 29, 2018
Member

As a rule, human beings find it easier to understand who a message was sent from or sent to if the name of the person is given than if an ID number is given. Please use the names.

Sources/Profile-Actions.php Show resolved Hide resolved
* @param bool $return_config Whether or not to return the config_vars array
* @return void|array Returns nothing or returns the $config_vars array if $return_config is true
*/
function ModifyPolicySettings($return_config = false)

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 23, 2018
Member

This function needs some logic to ensure that enable_policy_function cannot be set to true unless policy_version and any other necessary values are defined.

Right now, if the admin makes the mistake of enabling the policy function without setting up everything else first, the admin will be redirected on every page load to the page where he is prompted to accept the policy. But the admin will be unable to do so because there is no policy to accept, and so he will be stuck in an endless loop and will be unable to fix his forum except by editing his database directly.

This comment has been minimized.

@albertlast

albertlast Oct 28, 2018
Author Collaborator

Since i check everywhere if enabled_policy_function is true:

if (!empty($modSettings['enable_policy_function']) && !$user_info['is_guest'] && !$user_info['is_admin'])

I see no need here todo any checks in the settings.

Sources/ManageSettings.php Outdated Show resolved Hide resolved
@@ -50,6 +50,10 @@ function Register($reg_errors = array())
$context['require_agreement'] = !empty($modSettings['requireAgreement']);
$context['registration_passed_agreement'] = !empty($_SESSION['registration_agreed']);
$context['show_coppa'] = !empty($modSettings['coppaAge']);

// Do we nee them to agree to the policy agreement, secondly?
$context['require_policy'] = !empty($modSettings['enable_policy_function']);

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Oct 23, 2018
Member

Here would be another good place to add more logic to make sure that all the necessary variables are defined before requiring the policy.

Themes/default/Admin.template.php Show resolved Hide resolved
index.php Outdated Show resolved Hide resolved
albertlast added 6 commits Oct 28, 2018
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
function name is inet_dtop

Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
albertlast added 3 commits Oct 31, 2018
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
Signed-off-by: albertlast albertlast@hotmail.de
@Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Nov 6, 2018

Hey @albertlast. As you know, we are also working on adding code to SMF 2.0,16 to make it compliant with the GDPR. Some of the code in this PR can be used in 2.0.16, but other parts require a different approach in order to work within the confines of 2.0. So in order to maintain as much compatibility as possible between 2.0 and 2.1 and thereby make upgrading from 2.0 to 2.1 easier, my plan is to make my own local copy of this PR, integrate the changes we're making in 2.0 into it, and then push it all up as a new PR for 2.1. Once that is ready, I'll close this PR (and #5081, since it will also become obsolete) so that we can focus on testing the final version and get this task done. In the meantime, I am going to leave this PR open.

@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Nov 6, 2018

For better testing reason i would like to merge a newer version of smf into this pr,
but i'm not sure if the backwards transfer is than more complex.

Yes feel free to adapt this pr.

I did some poc with possibilty to generate the csv directly from the database side,
but the stuff feels not right and only works with postgres...

@albertlast
Copy link
Collaborator Author

@albertlast albertlast commented Nov 27, 2018

@Sesquipedalian for 2.1 we could also try to use generators: http://php.net/manual/en/language.generators.overview.php
this was added by php 5.5,
but i see no big problem to provide this function only with php 5.5+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants