Add GDPR script #649

Zlopez · 2018-10-29T14:45:05Z

Add SAR (Subject Access Requests) GDPR (General Data Protection Regulation)
script that could be used to gather user data from database.
This script currently takes data from users and
social_auth_usersocialauth table and saves them in json format.

Signed-off-by: Michal Konečný [email protected]

codecov-io · 2018-10-29T14:54:36Z

Codecov Report

Merging #649 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   95.76%   95.81%   +0.04%     
==========================================
  Files          56       57       +1     
  Lines        2835     2864      +29     
  Branches      387      391       +4     
==========================================
+ Hits         2715     2744      +29     
  Misses         84       84              
  Partials       36       36

Impacted Files	Coverage Δ
anitya/db/models.py	`100% <100%> (ø)`	⬆️
anitya/sar.py	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f781f2...456af31. Read the comment docs.

bowlofeggs · 2018-11-05T19:09:28Z

anitya/db/models.py

@@ -948,6 +948,31 @@ def get_id(self):
        """
        return six.text_type(self.id)

+    def __json__(self, detailed=False):


It looks like detailed does not do anything (and it is also undocumented).

Copied the method from other class and didn't noticed this parameter. Will remove.

bowlofeggs · 2018-11-05T19:10:51Z

anitya/sar.py

+from anitya.config import config
+from anitya import db
+
+LOG = logging.getLogger('anitya')


In Python, naming a variable in all caps conventionally means that it is a constant. The logger isn't a constant, so I recommend naming it _log (since it is also meant to be private to this module).

Copied from anitya_cron.py script. Will change.

bowlofeggs · 2018-11-05T19:12:59Z

anitya/sar.py

+
+    with open(FILENAME, 'w') as fd:
+        LOG.info('Dump data to json {}'.format(users_list))
+        json.dump(users_list, fd)


Note that the SAR data must be written to stdout as documented here:

https://fedora-infra-docs.readthedocs.io/en/latest/sysadmin-guide/sops/gdpr_sar.html#script

I thought the output is sar_output_file environment variable. If it should only print on stdout it is much easier to test.

bowlofeggs · 2018-11-05T19:13:37Z

anitya/tests/test_sar.py

+        sar.main()
+
+        testdata = open(sar.FILENAME).read()
+        print(testdata)


Stray print statement.

Will remove.

bowlofeggs · 2018-11-05T19:14:14Z

anitya/tests/test_sar.py

@@ -0,0 +1,137 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright © 2014  Red Hat, Inc.


Copied from another file. Didn't noticed. Will change.

jeremycline · 2018-11-06T20:01:06Z

anitya/sar.py

+
+    if SAR_EMAIL:
+        _log.debug('Find users by e-mail {}'.format(SAR_EMAIL))
+        users = users + session.query(db.User).filter(db.User.email == SAR_EMAIL).all()


You can write this as db.User.query..filter(db.User.email == SAR_EMAIL).all() or even db.User.query.filter_by(email=SAR_EMAIL).all().

Didn't know you can write it like this. It is more readable. I will change it.

jeremycline · 2018-11-06T20:03:38Z

anitya/db/models.py

@@ -954,6 +954,31 @@ def get_id(self):
        """
        return six.text_type(self.id)

+    def __json__(self):


I know this is copied from other model, but I do not think the name of this method conveys what it does. It's not a "real" dunder method, nor is it actually returning JSON. as_dict or to_dict or something is clearer.

Yeah, you are right.

jeremycline · 2018-11-06T20:07:59Z

anitya/tests/test_sar.py

+
+        out, err = self.capsys.readouterr()
+
+        self.assertTrue('"username": "' + user.username + '"' in out)


Using a format string here would make it easier to read:

Suggested change

self.assertTrue('"username": "' + user.username + '"' in out)

self.assertTrue('"username": "{}"'.format(user.username) in out)

I will change this.

Zlopez · 2018-11-22T09:03:12Z

@bowlofeggs is this looking good to you now?

bowlofeggs · 2018-11-26T14:30:25Z

anitya/tests/test_sar.py

+        out, err = self.capsys.readouterr()
+        print(out)
+
+        self.assertTrue('' in out)


This will be True for any str:

>>> '' in 'hello' True

I recommend asserting that the output is exactly the JSON string that is expected (would that be []?)

Thanks for noticing. I will change this to work with json.

bowlofeggs · 2018-11-26T14:32:02Z

anitya/tests/test_sar.py

+        self.assertTrue('"username": "{}"'.format(user.username) in out)
+        self.assertTrue('"email": "{}"'.format(user.email) in out)
+        self.assertFalse('"username": "{}"'.format(user2.username) in out)
+        self.assertFalse('"email": "{}"'.format(user2.email) in out)


Rather than spot checking strings, why not load the output with json.loads() and asserting that it is exactly the expected structure, including all keys?

Copied this from some other test, but you are right, it will be better to check against expected JSON. I will change this.

bowlofeggs · 2018-11-26T14:32:25Z

anitya/tests/test_sar.py

+        out, err = self.capsys.readouterr()
+
+        self.assertTrue('"username": "{}"'.format(user.username) in out)
+        self.assertTrue('"email": "{}"'.format(user.email) in out)


Similarly here, it would be good to assert that the output is valid JSON and has all the expected keys.

bowlofeggs · 2018-11-26T14:33:36Z

anitya/sar.py

+_log = logging.getLogger('anitya')
+
+SAR_USERNAME = os.getenv('SAR_USERNAME')
+SAR_EMAIL = os.getenv('SAR_EMAIL')


It would be easier to test this if these lines were part of main. Then you can use mock to set the environment variables and assert that these lines use os.getenv() correctly.

Yes, you are right. Thanks for noticing.

Zlopez · 2018-11-27T08:06:47Z

@bowlofeggs I addressed your changes and did a squash.

Zlopez · 2018-11-27T08:18:50Z

Rebased with latest master

bowlofeggs · 2018-11-27T19:32:25Z

anitya/tests/test_sar.py

+        }]
+
+        with mock.patch('os.getenv') as mock_getenv:
+            mock_getenv.side_effect = mock_getenv_email


Why not use mock.patch.dict() here instead? It's less code for you to maintain and is clearer for readers.

Didn't thought about this. You are right, it will be clearer.

bowlofeggs · 2018-11-27T19:33:30Z

anitya/tests/test_sar.py

+            }
+        ]
+
+        sar.SAR_USERNAME = 'user'


This line doesn't seem needed anymore.

Probably missed it.

Zlopez · 2018-11-28T09:21:15Z

I fixed the conflict and addressed the last suggestions.
There are still failing tests, but these will be fixed by #667

bowlofeggs · 2018-11-28T14:41:30Z

Cool, let me know once tests pass.

Add SAR (Subject Access Requests) GDPR (General Data Protection Regulation) script that could be used to gather user data from database. This script currently takes data from users and social_auth_usersocialauth table and saves them in json format. Signed-off-by: Michal Konečný <[email protected]>

Zlopez · 2018-11-28T15:00:52Z

@bowlofeggs
Tests are working again. \o/

bowlofeggs suggested changes Nov 5, 2018

View reviewed changes

Zlopez force-pushed the gdpr branch from c7dbdf2 to d7b36f3 Compare November 6, 2018 13:38

jeremycline reviewed Nov 6, 2018

View reviewed changes

bowlofeggs suggested changes Nov 26, 2018

View reviewed changes

Zlopez force-pushed the gdpr branch from 8f82262 to 7a6e167 Compare November 27, 2018 08:05

Zlopez force-pushed the gdpr branch from 7a6e167 to ab361dd Compare November 27, 2018 08:07

bowlofeggs previously approved these changes Nov 27, 2018

View reviewed changes

Zlopez dismissed bowlofeggs’s stale review via 0af236a November 28, 2018 09:19

Zlopez force-pushed the gdpr branch from ab361dd to 0af236a Compare November 28, 2018 09:19

Zlopez force-pushed the gdpr branch from 0af236a to 456af31 Compare November 28, 2018 14:49

bowlofeggs approved these changes Nov 29, 2018

View reviewed changes

mergify bot merged commit ef735f7 into fedora-infra:master Nov 29, 2018

Zlopez deleted the gdpr branch February 22, 2019 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GDPR script #649

Add GDPR script #649

Zlopez commented Oct 29, 2018

codecov-io commented Oct 29, 2018 •

edited

Loading

bowlofeggs Nov 5, 2018

Zlopez Nov 6, 2018

bowlofeggs Nov 5, 2018

Zlopez Nov 6, 2018

bowlofeggs Nov 5, 2018

Zlopez Nov 6, 2018

bowlofeggs Nov 5, 2018

Zlopez Nov 6, 2018

bowlofeggs Nov 5, 2018

Zlopez Nov 6, 2018

jeremycline Nov 6, 2018

Zlopez Nov 7, 2018

jeremycline Nov 6, 2018

Zlopez Nov 7, 2018

jeremycline Nov 6, 2018

Zlopez Nov 7, 2018

Zlopez commented Nov 22, 2018

bowlofeggs Nov 26, 2018

Zlopez Nov 26, 2018

bowlofeggs Nov 26, 2018

Zlopez Nov 26, 2018

bowlofeggs Nov 26, 2018

bowlofeggs Nov 26, 2018

Zlopez Nov 26, 2018

Zlopez commented Nov 27, 2018 •

edited

Loading

Zlopez commented Nov 27, 2018

bowlofeggs Nov 27, 2018

Zlopez Nov 28, 2018

bowlofeggs Nov 27, 2018

Zlopez Nov 28, 2018

Zlopez commented Nov 28, 2018

bowlofeggs commented Nov 28, 2018

Zlopez commented Nov 28, 2018


		out, err = self.capsys.readouterr()

		self.assertTrue('"username": "' + user.username + '"' in out)

	self.assertTrue('"username": "' + user.username + '"' in out)
	self.assertTrue('"username": "{}"'.format(user.username) in out)

Add GDPR script #649

Add GDPR script #649

Conversation

Zlopez commented Oct 29, 2018

codecov-io commented Oct 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Nov 27, 2018 • edited Loading

Zlopez commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Nov 28, 2018

bowlofeggs commented Nov 28, 2018

Zlopez commented Nov 28, 2018

codecov-io commented Oct 29, 2018 •

edited

Loading

Zlopez commented Nov 27, 2018 •

edited

Loading