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 ability to create domain-scoped API keys #28493

Merged
merged 14 commits into from
Oct 8, 2020
42 changes: 42 additions & 0 deletions corehq/apps/api/tests/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ def setUpClass(cls):
cls.password = '***'
cls.user = WebUser.create(cls.domain, cls.username, cls.password, None, None)
cls.api_key, _ = HQApiKey.objects.get_or_create(user=WebUser.get_django_user(cls.user))
cls.domain_api_key, _ = HQApiKey.objects.get_or_create(user=WebUser.get_django_user(cls.user),
name='domain-scoped',
domain=cls.domain)

@classmethod
def tearDownClass(cls):
Expand Down Expand Up @@ -85,6 +88,19 @@ def test_auth_type_basic(self):

class LoginAndDomainAuthenticationTest(AuthenticationTestBase):

@classmethod
def setUpClass(cls):
super().setUpClass()
cls.domain2 = 'api-test-other'
cls.project2 = Domain.get_or_create_with_name(cls.domain2, is_active=True)
cls.user.add_domain_membership(cls.domain2, is_admin=True)
cls.user.save()

@classmethod
def tearDownClass(cls):
cls.project2.delete()
super().tearDownClass()

def test_login_no_auth_no_domain(self):
self.assertAuthenticationFail(LoginAndDomainAuthentication(), self._get_request())

Expand All @@ -94,6 +110,32 @@ def test_login_no_auth_with_domain(self):
def test_login_with_domain(self):
self.assertAuthenticationSuccess(LoginAndDomainAuthentication(),
self._get_request_with_api_key(domain=self.domain))
self.assertAuthenticationSuccess(LoginAndDomainAuthentication(),
self._get_request_with_api_key(domain=self.domain2))

def test_login_with_domain_key(self):
self.assertAuthenticationSuccess(
LoginAndDomainAuthentication(),
self._get_request(
self.domain,
HTTP_AUTHORIZATION=self._construct_api_auth_header(
self.username,
self.domain_api_key
)
)
)

def test_login_with_domain_key_wrong(self):
self.assertAuthenticationFail(
LoginAndDomainAuthentication(),
self._get_request(
self.domain2,
HTTP_AUTHORIZATION=self._construct_api_auth_header(
self.username,
self.domain_api_key
)
)
)

def test_login_with_wrong_domain(self):
project = Domain.get_or_create_with_name('api-test-fail', is_active=True)
Expand Down
15 changes: 15 additions & 0 deletions corehq/apps/settings/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,21 +263,31 @@ def __init__(self, **kwargs):


class HQApiKeyForm(forms.Form):
ALL_DOMAINS = '*'
name = forms.CharField()
ip_allowlist = SimpleArrayField(
forms.GenericIPAddressField(),
label=ugettext_lazy("Allowed IP Addresses (comma separated)"),
required=False
)
domain = forms.ChoiceField(
required=False,
help_text=ugettext_lazy("Limit the key's access to a single project space")
)

def __init__(self, *args, **kwargs):
self.couch_user = kwargs.pop('couch_user')
super().__init__(*args, **kwargs)

user_domains = self.couch_user.get_domains()
all_domains = (self.ALL_DOMAINS, _('All Domains'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: user-facing content usually says "project" or "project space" instead of "domain."

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. be674a9

self.fields['domain'].choices = [all_domains] + [(d, d) for d in user_domains]
self.helper = HQFormHelper()
self.helper.layout = Layout(
crispy.Fieldset(
ugettext_lazy("Add New API Key"),
crispy.Field('name'),
crispy.Field('domain'),
crispy.Field('ip_allowlist'),
),
hqcrispy.FormActions(
Expand All @@ -294,10 +304,15 @@ def create_key(self, user):
HQApiKey.objects.get(name=self.cleaned_data['name'], user=user)
raise DuplicateApiKeyName
except HQApiKey.DoesNotExist:
if self.cleaned_data['domain'] and self.cleaned_data['domain'] != self.ALL_DOMAINS:
domain = self.cleaned_data['domain']
else:
domain = ''
new_key = HQApiKey.objects.create(
name=self.cleaned_data['name'],
ip_allowlist=self.cleaned_data['ip_allowlist'],
user=user,
domain=domain,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if line 266 was changed to ALL_DOMAINS = '' could this be changed to

Suggested change
if self.cleaned_data['domain'] and self.cleaned_data['domain'] != self.ALL_DOMAINS:
domain = self.cleaned_data['domain']
else:
domain = ''
new_key = HQApiKey.objects.create(
name=self.cleaned_data['name'],
ip_allowlist=self.cleaned_data['ip_allowlist'],
user=user,
domain=domain,
new_key = HQApiKey.objects.create(
name=self.cleaned_data['name'],
ip_allowlist=self.cleaned_data['ip_allowlist'],
user=user,
domain=self.cleaned_data['domain'] or '',

Copy link
Member Author

Choose a reason for hiding this comment

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

good call 09281b0

)
return new_key

Expand Down
3 changes: 3 additions & 0 deletions corehq/apps/settings/templates/settings/user_api_keys.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script type="text/html" id="base-user-api-key-template">
<td data-bind="text: name"></td>
<td data-bind="text: key"></td>
<td data-bind="text: domain"></td>
<td data-bind="text: ip_allowlist"></td>
<td data-bind="text: created"></td>
<td>
Expand Down Expand Up @@ -59,7 +60,9 @@ <h3>
<script type="text/html" id="new-user-api-key-template">
<td data-bind="text: name"></td>
<td data-bind="text: key"></td>
<td data-bind="text: domain"></td>
<td data-bind="text: ip_allowlist"></td>
<td data-bind="text: created"></td>
<td></td>
</script>
{% endblock %}
7 changes: 5 additions & 2 deletions corehq/apps/settings/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ def column_names(self):
return [
_("Name"),
_("API Key"),
_("Domain"),
_("IP Allowlist"),
_("Created"),
_("Delete"),
Expand All @@ -547,6 +548,7 @@ def paginated_list(self):
"id": api_key.id,
"name": api_key.name,
"key": redacted_key,
"domain": api_key.domain or _('All Domains'),
"ip_allowlist": (
", ".join(api_key.ip_allowlist)
if api_key.ip_allowlist else _("All IP Addresses")
Expand All @@ -563,8 +565,8 @@ def post(self, *args, **kwargs):

def get_create_form(self, is_blank=False):
if self.request.method == 'POST' and not is_blank:
return HQApiKeyForm(self.request.POST)
return HQApiKeyForm()
return HQApiKeyForm(self.request.POST, couch_user=self.request.couch_user)
return HQApiKeyForm(couch_user=self.request.couch_user)

def get_create_item_data(self, create_form):
try:
Expand All @@ -577,6 +579,7 @@ def get_create_item_data(self, create_form):
'id': new_api_key.id,
'name': new_api_key.name,
'key': f"{new_api_key.key} ({copy_key_message})",
"domain": new_api_key.domain or _('All Domains'),
'ip_allowlist': new_api_key.ip_allowlist,
'created': new_api_key.created.isoformat()
},
Expand Down
8 changes: 8 additions & 0 deletions corehq/apps/users/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,11 @@ class DomainPermissionsMirrorAdmin(admin.ModelAdmin):


admin.site.register(DomainPermissionsMirror, DomainPermissionsMirrorAdmin)


class HQApiKeyAdmin(admin.ModelAdmin):
list_display = ['user', 'name', 'created', 'domain']
list_filter = ['created', 'domain']


admin.site.register(HQApiKey, HQApiKeyAdmin)
29 changes: 21 additions & 8 deletions corehq/apps/users/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,30 @@ def require_api_permission(permission, data=None, login_decorator=login_and_doma
permissions_to_check = {permission, api_access_permission}

def permission_check(request, domain):
if getattr(request, 'api_key', None) and request.api_key.role:
role = request.api_key.role
# first check user permissions and return immediately if not valid
user_has_permission = all(
request.couch_user.has_permission(domain, p, data=data)
for p in permissions_to_check
)
if not user_has_permission:
return False

# then check domain and role scopes, if present
api_key = getattr(request, 'api_key', None)

if not api_key:
return True # only api keys support additional checks
elif api_key.role:
return (
role.domain == domain
and all(request.couch_user.has_permission(domain, p, data=data)
for p in permissions_to_check)
and all(role.permissions.has(p, data) for p in permissions_to_check)
api_key.role.domain == domain and
czue marked this conversation as resolved.
Show resolved Hide resolved
all(api_key.role.permissions.has(p, data) for p in permissions_to_check)
)
elif api_key.domain:
# we've already checked for user and role permissions so all that's left is domain matching
return domain == api_key.domain
else:
return all(request.couch_user.has_permission(domain, p, data=data)
for p in permissions_to_check)
# unscoped API key defaults to user permissions
return True

return require_permission_raw(
None, login_decorator,
Expand Down
18 changes: 18 additions & 0 deletions corehq/apps/users/migrations/0024_hqapikey_domain.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 2.2.13 on 2020-09-04 14:25

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('users', '0023_hqapikey_role_id'),
]

operations = [
migrations.AddField(
model_name='hqapikey',
name='domain',
field=models.CharField(blank=True, default='', max_length=255),
),
]
3 changes: 3 additions & 0 deletions corehq/apps/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2998,6 +2998,7 @@ class HQApiKey(models.Model):
name = models.CharField(max_length=255, blank=True, default='')
created = models.DateTimeField(default=timezone.now)
ip_allowlist = ArrayField(models.GenericIPAddressField(), default=list)
domain = models.CharField(max_length=255, blank=True, default='')
role_id = models.CharField(max_length=40, blank=True, default='')

class Meta(object):
Expand All @@ -3022,4 +3023,6 @@ def role(self):
return UserRole.get(self.role_id)
except ResourceNotFound:
logging.exception('no role with id %s found in domain %s' % (self.role_id, self.domain))
elif self.domain:
return CouchUser.from_django_user(self.user).get_domain_membership(self.domain).role
Copy link
Contributor

Choose a reason for hiding this comment

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

When enterprise permissions are on, this will grant access to both their domain and any controlled domain (so, COVID state-level users will have access to all county domains). That's probably what you want, just wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't consider this use case and don't know much about how the enterprise permissions work to have an opinion. Do you think it's the right behavior given the expectations of those roles as they're being used by the team?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think the current behavior matches what the team will expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said that, I might as well verify that assumption. I'll report back if the current behavior is not what they expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you report back either way? Would feel better about having the loop closed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current code is the correct behavior.

return None