add ability to create domain-scoped API keys #28493

czue · 2020-09-10T09:55:53Z

Had this almost done prior to 👶 and wanted to get it out before the 🕸️ get too dusty...

SUMMARY

This adds a new "domain" option to API keys to scope the key to only have permission for that domain. All existing keys will be assigned to the "all domains" scope.

Here's the UI:

FEATURE FLAG

RISK ASSESSMENT / QA PLAN

I feel good about test coverage and local testing, though it could make sense to do a dry run on staging. I may ask to hand that off if someone wants to make the case for it given life circumstance.

There is also a minor, innocuous DB migration.

PRODUCT DESCRIPTION

See screenshot.

corehq/apps/users/decorators.py

czue · 2020-09-10T09:57:03Z

cc @dimagi/product

snopoke

Are there still plans to allow selecting a role for the API key via the UI? As I see it that would also restrict an API key to a specific domain correct?

I imagine that you would want to have one or the other (role or domain) but not both.

czue · 2020-09-10T11:49:38Z

Are there still plans to allow selecting a role for the API key via the UI? As I see it that would also restrict an API key to a specific domain correct?

I imagine that you would want to have one or the other (role or domain) but not both.

Yep this was planned. I was imagining a second dropdown, and then when you choose a domain you can also (optionally) choose a role, where the roles are dynamically pulled based the domain. But to do that in the UI was more than I could bite off before disappearing. Then I was thinking of adding an assertion in the model that if the role is set then the domain must match the role (or we could require only one is set if that seems preferable - though setting the domain makes it a bit easier in the django admin and other places)

orangejenny · 2020-09-10T13:14:50Z

corehq/apps/users/models.py

@@ -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


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.

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?

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

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

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

The current code is the correct behavior.

orangejenny · 2020-09-10T13:16:49Z

corehq/apps/settings/forms.py

        super().__init__(*args, **kwargs)

+        user_domains = self.couch_user.get_domains()
+        all_domains = (self.ALL_DOMAINS, _('All Domains'))


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

good catch. be674a9

millerdev · 2020-09-10T22:10:34Z

corehq/apps/settings/forms.py

+            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,


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 '',

good call 09281b0

nicknestle · 2020-09-11T15:51:25Z

Is this going through QA? I'm nervous about our COVID projects being affected if this should go wrong given they all rely heavily on API integrations that cut across multiple domains. Is there a way we can confirm this won't break existing integrations and can we have a plan that when this is deployed we are able to actively monitor APIs in COVID domains to ensure nothing has gone wrong?

kcowger · 2020-09-11T20:24:46Z

Is this going through QA? I'm nervous about our COVID projects being affected if this should go wrong given they all rely heavily on API integrations that cut across multiple domains. Is there a way we can confirm this won't break existing integrations and can we have a plan that when this is deployed we are able to actively monitor APIs in COVID domains to ensure nothing has gone wrong?

I'm seconding this. I think we should be careful around APIs given recent issues

czue · 2020-09-12T04:45:36Z

Is this going through QA?

If we want to put this through QA, someone else will need to run that process as I'm on leave.

I'm nervous about our COVID projects being affected if this should go wrong given they all rely heavily on API integrations that cut across multiple domains. Is there a way we can confirm this won't break existing integrations and can we have a plan that when this is deployed we are able to actively monitor APIs in COVID domains to ensure nothing has gone wrong?

I have high confidence in the test coverage to catch any issues at the API access level. Monitoring on deploy sounds good as an extra precaution. My opinion is that this change is quite low risk (but happy for others to make the call to do more work to get more confidence)

millerdev · 2020-09-14T14:58:48Z

@czue will a quick and easy rollback be possible if things go badly? Maybe it would make sense to split out the db migration into a separate PR (having ~no risk) from the API behavior change to make a rollback less risky?

czue · 2020-09-15T07:07:38Z

@millerdev you're saying we merge and deploy the migration first, then deploy the UI and api changes separately so the latter can be rolled back if there are issues?

That sounds fine to me, though in a separate offline thread it sounds like the US team wants a more formal QA process to be run. Hopefully someone will be able to pick that up in my absence, else this PR might just get closed till I'm back in a few months. 😞

Ben-Talbot · 2020-09-16T20:45:14Z

Apologies all! My Github login is my personal gmail and I wasn't on the lookout for work related items with this email address. I'll update this so that going forward, these kind of messages will be in my work email.

Acknowledging that Cory will not be able to be the POC for any formal testing, I'd still be happy to take a pass on APIs on staging prior to release. We did something similar when Farid first unveiled the IP whitelist feature a few months back. When this is on staging, could someone please submit the QA request form (linked) and we can perform a review. Thanks all!

https://docs.google.com/forms/d/e/1FAIpQLSdSNi4r1UAenewXBvhL6qcDOfR9KnYYzFJBQd8uz6LOHW6ptg/viewform

czue added 9 commits September 10, 2020 10:41

add (non-functional) domain selection to api key UI

a76af8a

add domain to api keys

ca0284d

add db migration

0b38aea

add failing test for domain-only scoped api key access

eb5cb7c

refactor permission check and support domain-scoped keys

061d9fd

populate domain on form and save on api key model

e01c200

add api key admin page

9b468a1

bump domain above whitelist in the form

d3fe422

add domain to the UI (and fix dangling column on new keys)

2263690

czue requested review from esoergel and orangejenny as code owners September 10, 2020 09:55

stickler-ci reviewed Sep 10, 2020

View reviewed changes

corehq/apps/users/decorators.py Outdated Show resolved Hide resolved

czue requested review from proteusvacuum, millerdev, ctsims and snopoke September 10, 2020 09:56

czue added the product/all-users-all-environments Change impacts all users on all environments label Sep 10, 2020

czue added the reindex/migration Reindex or migration will be required during or before deploy label Sep 10, 2020

feed the stickler

168076b

snopoke approved these changes Sep 10, 2020

View reviewed changes

orangejenny reviewed Sep 10, 2020

View reviewed changes

proteusvacuum approved these changes Sep 10, 2020

View reviewed changes

millerdev approved these changes Sep 10, 2020

View reviewed changes

czue added 2 commits September 11, 2020 09:25

domain --> project

be674a9

simplify logic around all domains

09281b0

biyeun approved these changes Sep 11, 2020

View reviewed changes

czue added the Open for review: do not merge A work in progress label Sep 15, 2020

kaapstorm added 2 commits October 6, 2020 16:35

Merge branch 'master' into cz/role-ui

4521699

Update DB migration

2799e5b

kaapstorm mentioned this pull request Oct 7, 2020

Model & migration for domain-scoped API keys #28608

Merged

kaapstorm merged commit a52f934 into master Oct 8, 2020

kaapstorm deleted the cz/role-ui branch October 8, 2020 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to create domain-scoped API keys #28493

add ability to create domain-scoped API keys #28493

czue commented Sep 10, 2020

czue commented Sep 10, 2020

snopoke left a comment

czue commented Sep 10, 2020 •

edited

Loading

orangejenny Sep 10, 2020

czue Sep 11, 2020

orangejenny Sep 11, 2020

orangejenny Sep 11, 2020

czue Sep 12, 2020

orangejenny Sep 12, 2020

orangejenny Sep 10, 2020

czue Sep 11, 2020

millerdev Sep 10, 2020

czue Sep 11, 2020

nicknestle commented Sep 11, 2020

kcowger commented Sep 11, 2020

czue commented Sep 12, 2020

millerdev commented Sep 14, 2020

czue commented Sep 15, 2020

Ben-Talbot commented Sep 16, 2020

add ability to create domain-scoped API keys #28493

add ability to create domain-scoped API keys #28493

Conversation

czue commented Sep 10, 2020

SUMMARY

FEATURE FLAG

RISK ASSESSMENT / QA PLAN

PRODUCT DESCRIPTION

czue commented Sep 10, 2020

snopoke left a comment

Choose a reason for hiding this comment

czue commented Sep 10, 2020 • edited Loading

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

nicknestle commented Sep 11, 2020

kcowger commented Sep 11, 2020

czue commented Sep 12, 2020

millerdev commented Sep 14, 2020

czue commented Sep 15, 2020

Ben-Talbot commented Sep 16, 2020

czue commented Sep 10, 2020 •

edited

Loading