Merge lp:~roadmr/canonical-identity-provider/gdpr-report into lp:canonical-identity-provider/release

Proposed by Daniel Manrique on 2019-03-26
Status: Merged
Approved by: Daniel Manrique on 2019-03-27
Approved revision: 1683
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~roadmr/canonical-identity-provider/gdpr-report
Merge into: lp:canonical-identity-provider/release
Diff against target: 164 lines (+119/-1)
3 files modified
src/identityprovider/admin.py (+12/-1)
src/identityprovider/templates/admin/gdpr_report.html (+47/-0)
src/identityprovider/tests/test_admin.py (+60/-0)
To merge this branch: bzr merge lp:~roadmr/canonical-identity-provider/gdpr-report
Reviewer Review Type Date Requested Status
Maximiliano Bertacchini 2019-03-26 Approve on 2019-03-27
Review via email: mp+365134@code.launchpad.net

Commit message

    Add GDPR report admin action for accounts.

    The intent is to have a read-only, copy-pasteable view for GDPR requests.
    Currently the information to be reported is scattered between the Account
    change form and the auth logs changelist. The Account form contains most of the
    relevant data but since it's a form, it can't be cleanly copy-pasted.

    If more GDPR-relevant information is required, this view can easily be expanded
    to present that as well.

Description of the change

    Add GDPR report admin action for accounts.

    The intent is to have a read-only, copy-pasteable view for GDPR requests.
    Currently the information to be reported is scattered between the Account
    change form and the auth logs changelist. The Account form contains most of the
    relevant data but since it's a form, it can't be cleanly copy-pasted.

    If more GDPR-relevant information is required, this view can easily be expanded
    to present that as well.

To post a comment you must log in.
Maximiliano Bertacchini (maxiberta) wrote :

LGTM, but check a couple of questions inline. Thanks!

1683. By Daniel Manrique on 2019-03-27

Fix review comments

Daniel Manrique (roadmr) wrote :

Updated per review comments, thanks for your attention to deTALALAL :)

Maximiliano Bertacchini (maxiberta) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/identityprovider/admin.py'
--- src/identityprovider/admin.py 2018-05-29 19:27:59 +0000
+++ src/identityprovider/admin.py 2019-03-27 14:22:19 +0000
@@ -16,6 +16,7 @@
16from django.core import validators16from django.core import validators
17from django.db import connections17from django.db import connections
18from django.db.models.query import QuerySet18from django.db.models.query import QuerySet
19from django.shortcuts import render
19from django.template.defaultfilters import force_escape20from django.template.defaultfilters import force_escape
20from django.urls import reverse21from django.urls import reverse
21from django.utils.html import format_html22from django.utils.html import format_html
@@ -292,7 +293,7 @@
292293
293294
294class AccountAdmin(LargeTableMixin, admin.ModelAdmin):295class AccountAdmin(LargeTableMixin, admin.ModelAdmin):
295 actions = ['suspend']296 actions = ['suspend', 'gdpr_report']
296 inlines = [AccountPasswordInline, EmailAddressInline,297 inlines = [AccountPasswordInline, EmailAddressInline,
297 AuthenticationDeviceInline]298 AuthenticationDeviceInline]
298 readonly_fields = ('date_created',)299 readonly_fields = ('date_created',)
@@ -370,6 +371,16 @@
370371
371 suspend.short_description = "Suspend accounts"372 suspend.short_description = "Suspend accounts"
372373
374 def gdpr_report(self, request, queryset):
375 """Print a GDPR report with data we hold for the selected accounts."""
376 template = 'admin/gdpr_report.html'
377 context = {
378 'accounts': queryset.all()
379 }
380 return render(request, template, context)
381
382 gdpr_report.short_description = "GDPR report for accounts"
383
373384
374class APIUserAdminForm(forms.ModelForm):385class APIUserAdminForm(forms.ModelForm):
375386
376387
=== added file 'src/identityprovider/templates/admin/gdpr_report.html'
--- src/identityprovider/templates/admin/gdpr_report.html 1970-01-01 00:00:00 +0000
+++ src/identityprovider/templates/admin/gdpr_report.html 2019-03-27 14:22:19 +0000
@@ -0,0 +1,47 @@
1{% extends "admin/index.html" %}
2{% load i18n staticfiles %}
3
4{% comment %}
5Copyright 2019 Canonical Ltd. This software is licensed under the
6GNU Affero General Public License version 3 (see the file LICENSE).
7{% endcomment %}
8
9{% block breadcrumbs %}
10<div class="breadcrumbs"><a href="/admin/">
11 {% trans "Home" %}</a> &rsaquo;
12 <a href="/admin/identityprovider/">Identityprovider</a> &rsaquo;
13 {% trans "GDPR report" %}
14</div>
15{% endblock %}
16
17{% block content %}
18{% for account in accounts %}
19<h2>Account: {{account}}</h2>
20<h3>Personal data</h3>
21<table>
22 <tr><td>Display name: </td> <td>{{account.displayname}}</td></tr>
23 <tr><td>OpenID identifier:</td> <td>{{account.openid_identifier}}</td></tr>
24 <tr><td>Creation date: </td> <td>{{account.date_created}}</td></tr>
25 <tr><td>Last login: </td> <td>{{account.last_login}}</td></tr>
26 <tr><td>Status: </td> <td>{{account.get_status_display}}</td></tr>
27 <tr><td>Last status change: </td> <td>{{account.date_status_set}}</td></tr>
28 <tr><td>Account active: </td> <td>{{account.is_active}}</td></tr>
29 <tr><td>Creation source: </td> <td>{{account.creation_source}}</td></tr>
30 <tr><td>Launchpad name:</td> <td>{{account.person_name}}</td></tr>
31</table>
32<h3>E-mail addresses</h3>
33<ol>
34 {% for email in account.emailaddress_set.all %}
35 <li>{{email}} {% if email.is_preferred %}(preferred){% endif %}</li>
36 {% endfor %}
37</ol>
38<h3>Access logs</h3>
39<ol>
40 {% for authlog in account.authlog_set.all %}
41 <li>{{authlog}}</li>
42 {% endfor %}
43</ol>
44{% endfor %}
45{% endblock %}
46
47{% block sidebar %}{% endblock %}
048
=== modified file 'src/identityprovider/tests/test_admin.py'
--- src/identityprovider/tests/test_admin.py 2018-05-29 19:27:59 +0000
+++ src/identityprovider/tests/test_admin.py 2019-03-27 14:22:19 +0000
@@ -54,6 +54,7 @@
54 EmailStatus,54 EmailStatus,
55)55)
56from identityprovider.tests.utils import SSOBaseTestCase56from identityprovider.tests.utils import SSOBaseTestCase
57from testing.helpers import assert_no_extra_queries_after
5758
5859
59class AdminTestCase(SSOBaseTestCase):60class AdminTestCase(SSOBaseTestCase):
@@ -733,3 +734,62 @@
733 for account in self.accounts:734 for account in self.accounts:
734 account.refresh_from_db()735 account.refresh_from_db()
735 self.assertEqual(account.status, AccountStatus.SUSPENDED)736 self.assertEqual(account.status, AccountStatus.SUSPENDED)
737
738 def test_gdpr_report_all_accounts(self):
739 action_data = {
740 'action': 'gdpr_report',
741 '_selected_action': [account.pk for account in self.accounts]
742 }
743 with self.assertTemplateUsed('admin/gdpr_report.html'):
744 response = self.client.post(
745 reverse('admin:identityprovider_account_changelist'),
746 action_data, follow=True)
747 for account in self.accounts:
748 self.assertContains(response, account.displayname)
749 self.assertContains(
750 response, "%s (preferred)" % account.preferredemail)
751 for email in account.emailaddress_set.all():
752 self.assertContains(response, email)
753
754 def test_gdpr_query_count(self):
755 # We only test addition of e-mails to existing accounts;
756 # query stability for multiple accounts is not possible without
757 # significant refactoring of Account.user() but is also not a big
758 # performance hit because it's only one extra query per account.
759 def add_emails():
760 for account in self.accounts:
761 self.factory.make_email_for_account(account)
762
763 def get_gdpr_report():
764 action_data = {
765 'action': 'gdpr_report',
766 '_selected_action': [account.pk for account in self.accounts]
767 }
768 self.client.post(
769 reverse('admin:identityprovider_account_changelist'),
770 action_data, follow=True)
771
772 assert_no_extra_queries_after(add_emails, get_gdpr_report)
773
774 def test_gdpr_query_count_authlogs(self):
775 def add_authlogs():
776 for account in self.accounts:
777 for _ in range(10):
778 account.authlog_set.create(
779 log_type=1, login_email=account.preferredemail.email,
780 referer="foo", remote_ip='1.2.3.4',
781 user_agent="firefox")
782 account.save()
783
784 def get_gdpr_report():
785 action_data = {
786 'action': 'gdpr_report',
787 '_selected_action': [account.pk for account in self.accounts]
788 }
789 self.client.post(
790 reverse('admin:identityprovider_account_changelist'),
791 action_data, follow=True)
792 # Add some auth logs first
793 add_authlogs()
794 # Now ensure add_authlogs again doesn't result in extra queries
795 assert_no_extra_queries_after(add_authlogs, get_gdpr_report)