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

[couch to sql] UserRole & Permissions: Part I - sql models + populate command #29688

Merged
merged 39 commits into from May 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
1ffd617
initial models
snopoke May 4, 2021
633e5bd
sync code + tests
snopoke May 5, 2021
73a8b10
update indexes & field constraints
snopoke May 7, 2021
5e88858
add creation / modification dates
snopoke May 7, 2021
6e06b68
simplify by removing methods
snopoke May 7, 2021
514b828
get assignable_by in one query
snopoke May 7, 2021
d61617d
lint
snopoke May 7, 2021
70d9009
less ambiguous python permission info model
snopoke May 10, 2021
83d4349
lint
snopoke May 10, 2021
adfbe98
add sql role to domain deletion and domain dump
snopoke May 10, 2021
36142fc
add extra models to deletion operation
snopoke May 10, 2021
c08cd3d
add check constraint to RolePermissions table
snopoke May 11, 2021
f8c8ed4
set permission table name to users_permission
snopoke May 12, 2021
b02a6d3
Merge branch 'master' into sk/sql-user-role
snopoke May 12, 2021
1944519
make upstream ID reference SQL id not couch id
snopoke May 14, 2021
8bea73a
fix assignable by
snopoke May 14, 2021
8db649c
fix set_permissions
snopoke May 14, 2021
210de3d
use correct var in diff message
snopoke May 14, 2021
a0ac09d
specify name for lists and submodules
snopoke May 14, 2021
5b35766
support for diffing plain list
snopoke May 14, 2021
5751a46
handle diffs as a list
snopoke May 14, 2021
2705645
Merge branch 'master' into sk/sql-user-role
snopoke May 14, 2021
2228470
lint
snopoke May 14, 2021
04caac3
add diff to populate_user_role
snopoke May 14, 2021
d15c5a1
Merge branch 'master' into sk/sql-user-role
snopoke May 17, 2021
4a96306
fix bad merge
snopoke May 17, 2021
5e14369
fix domain deletion
snopoke May 17, 2021
a4d4279
handle case where is not in the doc
snopoke May 18, 2021
c7cd3ce
remove None check in diff_attr
snopoke May 18, 2021
0ac117c
revert upstream_id to opaque couch ID
snopoke May 19, 2021
6ce7268
test 'remote roles' update + fix for assignable_by
snopoke May 19, 2021
4227259
Merge branch 'master' into sk/sql-user-role
snopoke May 19, 2021
c3859f1
fix tests after merge
snopoke May 19, 2021
e62d686
Merge branch 'master' into sk/sql-user-role
snopoke May 24, 2021
be4eb1e
Merge branch 'master' into sk/sql-user-role
snopoke May 25, 2021
6b9597a
remove referenced to removed permissions
snopoke May 25, 2021
d0cbb6a
validate permission info
snopoke May 25, 2021
c56f409
enforce permissions unique per role
snopoke May 25, 2021
150a5c5
comments
snopoke May 25, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -53,6 +53,18 @@ def diff_couch_and_sql(cls, couch, sql):
"""
raise NotImplementedError()

@classmethod
def get_filtered_diffs(cls, couch, sql):
diffs = cls.diff_couch_and_sql(couch, sql)
if isinstance(diffs, list):
diffs = list(filter(None, diffs))
return diffs

@classmethod
def get_diff_as_string(cls, couch, sql):
diffs = cls.get_filtered_diffs(couch, sql)
return "\n".join(diffs) if diffs else None

@classmethod
def diff_attr(cls, name, doc, obj, wrap_couch=None, wrap_sql=None, name_prefix=None):
"""
Expand Down Expand Up @@ -201,14 +213,6 @@ def handle(self, **options):
if not skip_verify:
logger.info(f"Found {self.diff_count} differences")

@classmethod
def get_diff_as_string(cls, couch, sql):
diff = cls.diff_couch_and_sql(couch, sql)
if isinstance(diff, list):
diffs = list(filter(None, diff))
diff = "\n".join(diffs) if diffs else None
return diff

def _verify_doc(self, doc, exit=True):
try:
couch_id_name = getattr(self.sql_class(), '_migration_couch_id_name', 'couch_id')
Expand Down
3 changes: 3 additions & 0 deletions corehq/apps/domain/deletion.py
Expand Up @@ -381,6 +381,9 @@ def _delete_demo_user_restores(domain_name):
ModelDeletion('users', 'Invitation', 'domain'),
ModelDeletion('users', 'DomainPermissionsMirror', 'source'),
ModelDeletion('users', 'UserReportingMetadataStaging', 'domain'),
ModelDeletion('users', 'SQLUserRole', 'domain', [
'RolePermission', 'RoleAssignableBy', 'SQLPermission'
]),
ModelDeletion('user_importer', 'UserUploadRecord', 'domain'),
ModelDeletion('zapier', 'ZapierSubscription', 'domain'),
ModelDeletion('dhis2', 'SQLDataValueMap', 'dataset_map__domain'),
Expand Down
32 changes: 30 additions & 2 deletions corehq/apps/domain/tests/test_delete_domain.py
Expand Up @@ -65,7 +65,6 @@
CaseRuleSubmission,
DomainCaseRuleRun,
)
from corehq.apps.domain.dbaccessors import get_docs_in_domain_by_class
from corehq.apps.domain.models import Domain, TransferDomainRequest
from corehq.apps.export.models.new import DataFile, EmailExportWhenDoneRequest
from corehq.apps.ivr.models import Call
Expand Down Expand Up @@ -100,7 +99,10 @@
from corehq.apps.smsforms.models import SQLXFormsSession
from corehq.apps.translations.models import SMSTranslations, TransifexBlacklist
from corehq.apps.userreports.models import AsyncIndicator
from corehq.apps.users.models import DomainRequest, Invitation
from corehq.apps.users.models import (
DomainRequest, Invitation, PermissionInfo, Permissions,
SQLUserRole, RolePermission, RoleAssignableBy
)
from corehq.apps.zapier.consts import EventTypes
from corehq.apps.zapier.models import ZapierSubscription
from corehq.blobs import CODES, NotFound, get_blob_db
Expand Down Expand Up @@ -875,6 +877,32 @@ def test_users_delete(self):
self._assert_users_counts(self.domain.name, 0)
self._assert_users_counts(self.domain2.name, 1)

def _assert_role_counts(self, domain_name, roles, permissions, assignments):
self.assertEqual(SQLUserRole.objects.filter(domain=domain_name).count(), roles)
self.assertEqual(RolePermission.objects.filter(role__domain=domain_name).count(), permissions)
self.assertEqual(RoleAssignableBy.objects.filter(role__domain=domain_name).count(), assignments)

def test_roles_delete(self):
for domain_name in [self.domain.name, self.domain2.name]:
role1 = SQLUserRole.objects.create(
domain=domain_name,
name="role1"
)
role = SQLUserRole.objects.create(
domain=domain_name,
name="role2"
)
role.set_permissions([
PermissionInfo(Permissions.view_reports.name, allow=PermissionInfo.ALLOW_ALL)
])
role.set_assignable_by([role1.id])
self._assert_role_counts(domain_name, 2, 1, 1)

self.domain.delete()

self._assert_role_counts(self.domain.name, 0, 0, 0)
self._assert_role_counts(self.domain2.name, 2, 1, 1)

def _assert_zapier_counts(self, domain_name, count):
self._assert_queryset_count([
ZapierSubscription.objects.filter(domain=domain_name),
Expand Down
1 change: 1 addition & 0 deletions corehq/apps/dump_reload/sql/dump.py
Expand Up @@ -126,6 +126,7 @@
FilteredModelIteratorBuilder('linked_domain.DomainLink', SimpleFilter('linked_domain')),
FilteredModelIteratorBuilder('linked_domain.DomainLinkHistory', SimpleFilter('link__linked_domain')),
FilteredModelIteratorBuilder('users.DomainPermissionsMirror', SimpleFilter('source')),
FilteredModelIteratorBuilder('users.SQLUserRole', SimpleFilter('domain')),
FilteredModelIteratorBuilder('locations.LocationFixtureConfiguration', SimpleFilter('domain')),
FilteredModelIteratorBuilder('commtrack.SQLCommtrackConfig', SimpleFilter('domain')),
FilteredModelIteratorBuilder('commtrack.SQLActionConfig', SimpleFilter('commtrack_config__domain')),
Expand Down
98 changes: 96 additions & 2 deletions corehq/apps/linked_domain/tests/test_update_roles.py
@@ -1,7 +1,13 @@
import uuid

from django.test import TestCase
from mock import patch

from corehq.apps.domain.shortcuts import create_domain
from corehq.apps.linked_domain.models import DomainLink
from corehq.apps.linked_domain.tests.test_linked_apps import BaseLinkedAppsTest
from corehq.apps.linked_domain.updates import update_user_roles
from corehq.apps.linked_domain.util import _clean_json
from corehq.apps.userreports.util import get_ucr_class_name
from corehq.apps.users.models import Permissions, UserRole

Expand All @@ -10,8 +16,6 @@ class TestUpdateRoles(BaseLinkedAppsTest):
@classmethod
def setUpClass(cls):
super(TestUpdateRoles, cls).setUpClass()
cls.linked_app.save()

cls.role = UserRole(
domain=cls.domain,
name='test',
Expand Down Expand Up @@ -104,3 +108,93 @@ def test_match_ids(self):
self.assertIsNotNone(roles.get('other_test'))
self.assertTrue(roles['other_test'].permissions.edit_web_users)
self.assertEqual(roles['other_test'].upstream_id, self.other_role.get_id)


class TestUpdateRolesRemote(TestCase):
@classmethod
def setUpClass(cls):
super(TestUpdateRolesRemote, cls).setUpClass()
cls.domain_obj = create_domain('domain')
cls.domain = cls.domain_obj.name

cls.linked_domain_obj = create_domain('domain-2')
cls.linked_domain = cls.linked_domain_obj.name

cls.domain_link = DomainLink.link_domains(cls.linked_domain, cls.domain)
cls.domain_link.remote_base_url = "http://other.org"
cls.domain_link.save()

@classmethod
def tearDownClass(cls):
cls.domain_link.delete()
cls.domain_obj.delete()
cls.linked_domain_obj.delete()
super(TestUpdateRolesRemote, cls).tearDownClass()

def setUp(self):
self.upstream_role1_id = uuid.uuid4().hex
self.role1 = UserRole(
domain=self.linked_domain,
name='test',
permissions=Permissions(
edit_data=True,
edit_reports=True,
view_report_list=[
'corehq.reports.DynamicReportmaster_report_id'
]
),
is_non_admin_editable=True,
upstream_id=self.upstream_role1_id
)
self.role1.save()

self.other_role = UserRole(
domain=self.linked_domain,
name='other_test',
permissions=Permissions(
edit_web_users=True,
view_locations=True,
),
assignable_by=[self.role1.get_id],
)
self.other_role.save()

def tearDown(self):
for role in UserRole.by_domain(self.linked_domain):
role.delete()
super(TestUpdateRolesRemote, self).tearDown()

@patch('corehq.apps.linked_domain.updates.remote_get_user_roles')
def test_update_remote(self, remote_get_user_roles):
remote_permissions = Permissions(
edit_data=False,
edit_reports=True,
view_report_list=['corehq.reports.static_report']
)
# sync with existing local role
remote_role1 = UserRole(
_id=self.upstream_role1_id,
name='test',
permissions=remote_permissions,
is_non_admin_editable=False
)
# create new role
remote_role_other = UserRole(
_id=uuid.uuid4().hex,
name="another",
permissions=Permissions(),
assignable_by=[self.upstream_role1_id]
)
remote_get_user_roles.return_value = [
_clean_json(role.to_json()) for role in [remote_role1, remote_role_other]
]

update_user_roles(self.domain_link)

roles = {r.name: r for r in UserRole.by_domain(self.linked_domain)}
self.assertEqual(3, len(roles))
self.assertEqual(roles['test'].permissions, remote_permissions)
self.assertEqual(roles['test'].is_non_admin_editable, False)
self.assertEqual(roles['another'].assignable_by, [self.role1.get_id])
self.assertEqual(roles['another'].permissions, Permissions())
self.assertEqual(roles['other_test'].assignable_by, [self.role1.get_id])
20 changes: 13 additions & 7 deletions corehq/apps/linked_domain/updates.py
Expand Up @@ -235,18 +235,24 @@ def update_user_roles(domain_link):
upstream_role = copy(role_def)
upstream_role.pop('_id')
upstream_role.pop('upstream_id')
upstream_role.pop('assignable_by') # handled below
role_json.update(upstream_role)
local_roles_by_upstream_id[role_json['upstream_id']] = role_json
UserRole.wrap(role_json).save()
role = UserRole.wrap(role_json)
role.save()
local_roles_by_upstream_id[role_json['upstream_id']] = role.to_json()

# Update assignable_by ids - must be done after main update to guarantee all local roles have ids
for role in local_roles_by_upstream_id.values():
if role['assignable_by']:
role['assignable_by'] = [
for role_def in master_results:
role_json = local_roles_by_upstream_id[role_def['_id']]
if role_def['assignable_by']:
role_json['assignable_by'] = [
local_roles_by_upstream_id[role_id]['_id']
for role_id in role['assignable_by']
for role_id in role_def['assignable_by']
]
UserRole.wrap(role).save()
UserRole.wrap(role_json).save()
elif role_json['assignable_by']:
role_json['assignable_by'] = None
UserRole.wrap(role_json).save()


def update_case_search_config(domain_link):
Expand Down
75 changes: 75 additions & 0 deletions corehq/apps/users/management/commands/populate_user_role.py
@@ -0,0 +1,75 @@
from corehq.apps.cleanup.management.commands.populate_sql_model_from_couch_model import PopulateSQLCommand
from corehq.apps.users.models import UserRole, Permissions
from corehq.apps.users.models_sql import (

Choose a reason for hiding this comment

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

F401 'corehq.apps.users.models_sql.SQLUserRole' imported but unused

migrate_role_assignable_by_to_sql,
migrate_role_permissions_to_sql,
SQLUserRole,
)


class Command(PopulateSQLCommand):
@classmethod
def couch_db_slug(cls):
return "users"

@classmethod
def couch_doc_type(self):
return 'UserRole'

@classmethod
def sql_class(self):
from corehq.apps.users.models import SQLUserRole

Choose a reason for hiding this comment

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

F811 redefinition of unused 'SQLUserRole' from line 3

return SQLUserRole

@classmethod
def commit_adding_migration(cls):
return "TODO: add once the PR adding this file is merged"

@classmethod
def diff_couch_and_sql(cls, couch, sql):
diffs = []
for field in UserRole._migration_get_fields():
diffs.append(cls.diff_attr(field, couch, sql))

couch_permissions = {
info.name: info
for info in Permissions.wrap(couch["permissions"]).to_list()
}
sql_permissions = {
info.name: info
for info in sql.permissions.to_list()
}

for name in sorted(set(couch_permissions) | set(sql_permissions)):
couch_permission = couch_permissions.get(name)
diffs.append(cls.diff_attr(
"allow",
couch_permission._asdict() if couch_permission else {},
sql_permissions.get(name),
name_prefix=f"permissions.{name}"
))

couch_assignable_by = couch["assignable_by"]
sql_assignable_by = list(sql.roleassignableby_set.values_list('assignable_by_role__couch_id', flat=True))
diffs.extend(cls.diff_lists(
"assignable_by",
sorted(couch_assignable_by) if couch_assignable_by else [],
sorted(sql_assignable_by),
))
return diffs

def update_or_create_sql_object(self, doc):
model, created = self.sql_class().objects.update_or_create(
couch_id=doc['_id'],
defaults={
"domain": doc.get("domain"),
"name": doc.get("name"),
"default_landing_page": doc.get("default_landing_page"),
"is_non_admin_editable": doc.get("is_non_admin_editable"),
"is_archived": doc.get("is_archived"),
"upstream_id": doc.get("upstream_id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

So for now upstream_id will contain the id of the relevant couch doc, and then when the code switches from reading couch to reading sql, this'll also get migrated to use SQL ids instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially. Both this and assignable_by use couch IDs currently. Also the role is referenced from the CouchUser object via the couch_id. Once the migration to SQL is done we can consider whether we want to try and get rid of the couch_id field and convert this to using the SQL IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI I've updated this so that upstream_id and assignable_by are the SQL IDs (not couch IDs)

Copy link
Contributor

Choose a reason for hiding this comment

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

And then reverted to Couch IDs in 0ac117c

})
couch_role = UserRole.wrap(doc)
migrate_role_permissions_to_sql(couch_role, model)
migrate_role_assignable_by_to_sql(couch_role, model)
return model, created