Support app-specific web apps permissions for web apps users #34421

orangejenny · 2024-04-11T19:15:28Z

Product Description

Overview

https://dimagi.atlassian.net/browse/USH-4289

This work overlaps somewhat with the Login As changes @MartinRiese is doing in #34351 This description is written assuming that those changes are complete. I'm planning to wait on QAing this PR until that work is complete.

Old behavior

There's a single permission in roles & permissions that controls access to web apps.

This permission is checked for the user who is logged into HQ. When Login As is used, permissions of the Login As user are ignored. Also, the web apps permission is not checked if the user logged into HQ is a mobile user. The default role for mobile workers doesn't even include web apps access (see here). All of this is controlled by the @require_cloudcare_access decorator, which is applied to cloudcare views.

In addition, access can be granted to specific applications in web apps using a bespoke set of models and UI:

This is based on mapping applications to user groups. User groups are only available for mobile users. Therefore, app-specific permissions are only available for mobile users.

Impetus for change

As part of the work to move web apps projects away from depending on linked mobile users, we need to support app-specific permissions for web users.

There's an alternative solution where we add support for web users to user groups. This is discussed in the ticket and linked documents, but in short, it was rejected in favor of integrating app-specific web apps permissions with our general roles & permissions framework.

New behavior

I'm clubbing a couple of related changes into this PR in the interest of getting holistic review. I'm happy to split things up if reviewers would find that helpful or believe it would be safer.

Add app-specific permissions to roles & permissions (first few commits)

This is a straightforward addition to our existing roles & permissions framework.

Add feature flag to hide old group-based behavior (next few commits)

Having two ways to assign app-specific permissions is confusing. Moving from group-based to role-based permissions requires human intervention. As a compromise, I'm adding a feature flag, marked deprecated, to gate the old UI. Migration 0059 turns this flag on for all domains that currently have an ApplicationAccess object that's restricting app access. I added a warning to the top of the old UI, but I'm not changing behavior at all. ApplicationAccess restrictions will continue to be applied as they are today, in addition to the new app-specific permissions.

~~3. Update default mobile worker role to access web apps, and remove mobile worker check (last few commits)~~

This is the broadest/scariest change. Pushing users to use roles to manage app-specific permissions, for both web and mobile workers, makes it more glaringly apparent that the existing web apps permission doesn't apply to mobile workers. To reduce ambiguity, I'm updating require_cloudcare_access to check the permission for both web and mobile users. To avoid a change in behavior, migration 0060 updates all of the default mobile worker roles to turn on web apps access.

How do these changes combine? I'm continuing to decorate cloudcare views with a permissions check on the HQ user, so if you log into HQ and don't have any cloudcare permissions, you always 403, no opportunity to login as someone else. If you can log into HQ and then use login as, that user's permissions are used to determine which apps you see. This does mean that you can log into HQ as a user who has access only to app A, and can login as a user who has access to app B.

Summary of behavior:

HQ user `access_web_apps`	Login As user `access_web_apps`	Behavior
False	no login as	403
app-specific	no login as	display apps HQ user has access to
	False	no apps displayed
	app-specific	display apps login as user has access to
	True	all apps displayed
True	no login as	all apps displayed
	False	no apps displayed
	app-specific	display apps login as user has access to
	True	all apps displayed

In addition to this, any legacy ApplicationAccess restrictions will continue to be applied, for backwards compatibility.

Feature Flag

Introduces WEB_APPS_PERMISSIONS_VIA_GROUPS, but most of this behavior is not gated.

Safety Assurance

Safety story

Permissions are always sensitive, and this does migrations ~~and changes default-type behavior~~, so it feels medium-high risk.

Automated test coverage

There are tests for roles and permissions.

QA Plan

https://dimagi.atlassian.net/browse/QA-6352

Migrations

The migrations in this code can be safely applied first independently of the code

No? The migrations are integrated with the code. I'm not terribly concerned that someone will add an ApplicationAccess object during deploy, after migration 0059 runs but before the new code that restricts the config UI based on that feature flag goes live.

I could pull the application access changes (the new feature flag, migration 0059, and the changes that restrict the old config UI) into a separate PR, or even two separate PRs (one for the flag + migration, a second for the UI restrictions). I'm not going to do that unless anyone else is concerned about the deploy window, because I think the risk here is low.

For the 0060 migration for the default mobile worker role, the current behavior is at best ambiguous and arguably incorrect, since a mobile worker with a role that doesn't have access to web apps still gets access to web apps. That makes me not concerned about default mobile worker roles getting updated during deploy.

Rollback instructions

This PR can be reverted after deploy with no further considerations

Rolling back will not be possible as soon as people start adding app-specific permissions to the database, as the wrap function will start to fail.

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

The rest of this was removed in #29767

…tial page data Privileges are already available on the front end.

…s for

esoergel

Exemplary PR, bravo! Great description, commit breakdown, etc

esoergel · 2024-04-11T21:03:10Z

corehq/apps/users/static/users/js/roles.js

@@ -267,22 +279,6 @@ hqDefine('users/js/roles',[
                        allowCheckboxId: null,
                        allowCheckboxPermission: null,
                    },
-                    {
-                        showOption: root.webAppsPrivilege,


I'm guessing this was for projects that have plans without access to web apps (is that Community?). Removing this restriction will mean that those projects will now be able to configure web apps permissions even when not applicable. Probably not a big deal, just wanted to check.

nevermind, I see it was functionally replaced with
{% if request|request_has_privilege:"CLOUDCARE" %}
below

esoergel · 2024-04-11T21:04:37Z

corehq/apps/users/templates/users/partials/edit_role_modal.html

+            <legend>
+              {% trans "Web Apps" %}
+            </legend>


Did this look like a decent target to extract into a template? Not sure how much it has in common with the other parameterized permissions

esoergel · 2024-04-11T21:07:51Z

corehq/apps/users/views/__init__.py

@@ -510,20 +510,6 @@ def can_restrict_access_by_location(self):
        return self.domain_object.has_privilege(
            privileges.RESTRICT_ACCESS_BY_LOCATION)

-    @property
-    @memoized
-    def release_management_privilege(self):


nice cleanup commit

esoergel · 2024-04-11T21:13:05Z

corehq/apps/users/templates/users/partials/edit_role_modal.html

@@ -556,3 +478,43 @@ <h4 class="modal-title" data-bind="text: modalTitle"></h4>
    </form>
  </div>
 </div>
+
+<script type="text/html" id="permission_all_selected_none">


lol, you're always a step ahead

esoergel · 2024-04-11T21:19:12Z

corehq/apps/users/migrations/0059_add_web_apps_groups_permission_flag.py

+    for access in ApplicationAccess.objects.filter(restrict=True):
+        WEB_APPS_PERMISSIONS_VIA_GROUPS.set(access.domain, enabled=True, namespace=NAMESPACE_DOMAIN)


are you anticipating this being fast enough to run during the deploy?

yeah, with only a couple hundred ApplicationAccess objects on prod this should be fast

esoergel · 2024-04-11T21:19:49Z

corehq/toggles/__init__.py

@@ -944,6 +944,13 @@ def _ensure_valid_randomness(randomness):
    """
 )

+WEB_APPS_PERMISSIONS_VIA_GROUPS = StaticToggle(


Might throw in a description pointing to the alternative, unless you did that on the page itself

esoergel · 2024-04-11T21:20:18Z

corehq/apps/cloudcare/templates/cloudcare/config.html

+    {% blocktrans %}
+      This feature is deprecated. Access to specific web apps can now be managed via
+      <a target="_blank" href="{{ roles_url }}">Roles & Permissions</a>.
+    {% endblocktrans %}


lol, yup there it is

It's a good point that the flag itself could be more descriptive, updated in 7aa89a6

esoergel · 2024-04-11T21:22:25Z

corehq/apps/users/migrations/0059_add_web_apps_groups_permission_flag.py

+
+@skip_on_fresh_install
+def enable_flag(apps, schema_editor):
+    for access in ApplicationAccess.objects.filter(restrict=True):


Is this as targeted as we can safely do? Like, would it make sense to check whether the domain has access to web apps or groups configured or some such? I guess that could also be a clean-up pass done separately later

I think it's pretty harmless, the view itself is still decorated to require the cloudcare privilege. The risks here are:

Block access to this page for a domain that should have it: I think this is unlikely, although I could remove the restrict=True filter.

Accidentally give access to a domain that doesn't need it: not a big deal, since right now all domains (with cloudcare privilege) can access this page

Accidentally give access to a domain that doesn't need it: not a big deal, since right now all domains (with cloudcare privilege) can access this page

Yeah, I agree - this is the situation I was thinking of. Fewer domains on this flag means that it's easier to eventually remove in the future, but I agree it's probably better to play it safe here and work on whittling down the list separately whenever we get around to it

esoergel · 2024-04-11T21:24:40Z

corehq/apps/users/migrations/0060_grant_web_apps_default_mobile_worker.py

+
+
+@skip_on_fresh_install
+def _update_default_mobile_worker_role(apps, schema_editor):


I'm also a bit nervous about the access change associated with this. There's always the possibility that there are mobile workers using roles other than the default and using web apps. I do think the current behavior is arguably a bug (if the role explicitly doesn't have web apps access...), so I think we have some wiggle room. Still, is there anything we could do to de-risk this, or get a sense of how many users it might realistically impact?

Point well taken. I'll do some digging on this and report back.

(You reviewed this a lot faster than I was expecting - thank you!)

reverted this change, see longer comment below

snopoke

This is a great change, thanks! If a web user doesn't have access to any apps can they still get the Web Apps page and use login as?

corehq/apps/cloudcare/views.py

corehq/tabs/tabclasses.py

MartinRiese · 2024-04-12T15:17:47Z

Can't add anything useful. But it is good learning to read through the PR and Ethan's comments.

Robert-Costello · 2024-04-12T15:23:36Z

Can't add anything useful. But it is good learning to read through the PR and Ethan's comments.

Same here. Reading through this PR and the comments is especially helpful since I'm starting on adjacent permissions work for tableau access.

This reverts commit 27c0cc3.

This reverts commit 16ddf89.

Even though this isn't checked, it's better for it to reflect actual behavior, which is that mobile workers all get web apps access.

orangejenny · 2024-04-18T17:15:28Z

I'm reverting the last few commits, which change the mobile worker default role and remove the is_commcare_user checks for web apps access, as I don't believe they're safe to make without substantially more due diligence and also likely a slower rollout that's explicitly communicated to users.

Many mobile workers have no role set. This makes sense, since the default role is a relatively recent addition and we're lazily migrating mobile workers to use it (see #32057). For those mobile workers, if you ask for their role, you get the default role back (see #32008), but directly accessing their permissions doesn't go through that default role:

> user = CouchUser.get_by_user_id('<a_user_id_with_no_role_set>')

> user.domain_membership.role_id   # no role set directly on the domain membership

> user.domain_membership.role  # access role on object, get back default role, great
UserRole(domain='3156-down', name='Mobile Worker Default')

> user.get_domain_membership('3156-down').role.permissions.access_web_apps  # default role can access web apps, great
True

> user.can_access_web_apps()  # checking permissions on user does not use default role, alas
False

That means the migration to add access_web_apps to all default mobile worker roles is insufficient to actually grant them access to web apps. We'd need to either do a non-lazy migration to assign roles to all mobile workers, or update permissions checking so that it respects the default role if no role is explicitly set. Both of those are out of scope.

millerdev · 2024-04-18T17:39:03Z

corehq/apps/users/migrations/0061_grant_web_apps_default_mobile_worker.py

Does removing this file have any implications for live environments (possibly staging)? Should the change be reversed?

Also, what happens when a future 0061 migration is added to this app? Will it appear to have been applied in any environment where this was applied? Maybe this has not been applied anywhere, in which case there is no concern.

Important questions, and yes, this migration had already been run on staging. I ran migrate users 0060 on staging to "undo" the migration from django's perspective, and then undid the changes in a shell (running the code that was in the migration but flipping access_web_apps to False).

esoergel · 2024-04-18T20:55:45Z

@orangejenny that behavior looks very bizarre to me. I just took a look and I think this is a bug, though one that shouldn't affect authorization. Check this out:

>>> user.can_access_web_apps()
False
>>> user.can_access_web_apps(self.domain)
True

That's the core of the problem, I believe. can_access_web_apps goes through __getattr__ and _get_perm_check_fn

commcare-hq/corehq/apps/users/models.py

Lines 1600 to 1615 in 31ddfc0

    
           def __getattr__(self, item): 
        
               if item.startswith('can_'): 
        
                   perm = item[len('can_'):] 
        
                   if perm: 
        
                       fn = self._get_perm_check_fn(perm) 
        
                       fn.__name__ = item 
        
                       return fn 
        
               raise AttributeError("'{}' object has no attribute '{}'".format( 
        
                   self.__class__.__name__, item)) 
        
           def _get_perm_check_fn(self, perm): 
        
               def fn(domain=None, data=None): 
        
                   domain = domain or getattr(self, 'current_domain', None) 
        
                   return self.has_permission(domain, perm, data) 
        
               return fn

So the two calls above resolve to

>>> user.has_permission(None, 'access_web_apps')
False
>>> user.has_permission(self.domain, 'access_web_apps')
True

That is, this will work if domain is passed to the can_ call, or if it is in a request context, where current_domain is populated. This is a gross inconsistency in the code.

esoergel · 2024-04-18T20:58:07Z

(I do still support your decision to separate the main changes from addressing this stuff though)

orangejenny · 2024-04-22T14:59:20Z

@esoergel OH. That makes sense. Yeah, my testing was bad. Which is a relief. But yeah, I think I'm going to keep those changes reverted, I'm just gun-shy about sweeping permissions changes.

USH is responsible for this deprecation.

This reverts commit 92099af.

millerdev · 2024-04-24T16:49:32Z

corehq/apps/linked_domain/updates.py

+            permissions.web_apps_list = [
+                downstream_id for downstream_id, upstream_id in app_id_map.items()
+                if upstream_id in permissions.web_apps_list
+            ]


I assume this new usage of app ids does not require any data migrations since this is a new feature that is only deployed on staging where old data either does not exist or does not matter. Is that right?

Yes, exactly.

esoergel

Good catch with the linked domains thing, that wasn't on my radar

esoergel · 2024-04-24T20:53:10Z

corehq/apps/cloudcare/views.py

+        def _can_access_web_app(user, domain, app):
+            if user.can_access_web_app(domain, app.get('copy_of', app.get('_id'))):
+                return True
+            if user.can_access_web_app(domain, app.get('upstream_app_id')):


Can you elaborate on this one? This is for the situation where there are two apps on the same domain, one linked to the other? Would these show up as different entries on the roles page where you select which apps are accessible?

esoergel · 2024-04-24T20:56:52Z

corehq/apps/linked_domain/tests/test_update_roles.py

+        role = UserRole.objects.get_by_domain(self.downstream_domain)[0]
+        self.assertListEqual(role.permissions.web_apps_list, [downstream_app.id])


Ah this clarifies it, thank you. In my words, it's that a role on an upstream domain with access to an app needs to be propagated to the downstream domain with the app access updated to the corresponding downstream app. Did I get that right?

yep, exactly

orangejenny added 7 commits April 10, 2024 17:36

Added web_apps_list to model

8e525f3

Made list of web apps available to javascript

08811d4

Removed unused initial page data

cd68c0f

The rest of this was removed in #29767

Added apps list to UI

3d01962

Pulled linked domain privileges from privileges module instead of ini…

3b3c0c0

…tial page data Privileges are already available on the front end.

Extracted template for the access all/none/selected UI

0b09e28

Updated web apps to filter out apps that user doesn't have permission…

703f1e0

…s for

orangejenny added awaiting QA QA in progress. Do not merge product/all-users-all-environments Change impacts all users on all environments Risk: Medium Change affects files that have been flagged as medium risk. labels Apr 11, 2024

orangejenny requested review from snopoke and millerdev April 11, 2024 19:15

orangejenny requested review from esoergel, MartinRiese, AddisonDunn, stephherbers, Robert-Costello, Jtang-1 and nospame as code owners April 11, 2024 19:15

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Apr 11, 2024

orangejenny force-pushed the jls/web-apps-specific-permissions branch from 8de9ec2 to bc74d48 Compare April 11, 2024 20:19

orangejenny added 3 commits April 11, 2024 16:20

Added flag to gate old behavior

d523a05

Moved legacy UI behind feature flag

060b090

Stopped allowing all mobile users access to web apps

16ddf89

orangejenny force-pushed the jls/web-apps-specific-permissions branch from bc74d48 to 16ddf89 Compare April 11, 2024 20:20

esoergel approved these changes Apr 11, 2024

View reviewed changes

Added description for WEB_APPS_PERMISSIONS_VIA_GROUPS flag

7aa89a6

snopoke reviewed Apr 12, 2024

View reviewed changes

corehq/apps/cloudcare/views.py Outdated Show resolved Hide resolved

corehq/tabs/tabclasses.py Outdated Show resolved Hide resolved

orangejenny added 4 commits April 17, 2024 14:41

Merge branch 'master' into jls/web-apps-specific-permissions

4b902dd

Revert "Removed leftover is_commcare_user check"

83fbe44

This reverts commit 27c0cc3.

Revert "Stopped allowing all mobile users access to web apps"

842c66a

This reverts commit 16ddf89.

Added access_web_apps to default mobile worker role

763cf34

Even though this isn't checked, it's better for it to reflect actual behavior, which is that mobile workers all get web apps access.

millerdev approved these changes Apr 18, 2024

View reviewed changes

orangejenny added 2 commits April 22, 2024 10:04

Added domain to can_access_any_web_apps call

f2e32d1

Updated message on legacy page

115c28b

Updated dropdown option text

8c64703

millerdev approved these changes Apr 22, 2024

View reviewed changes

orangejenny added 3 commits April 22, 2024 14:04

Merge branch 'master' into jls/web-apps-specific-permissions

d5cc983

Tagged new flag as USH

fa113b1

USH is responsible for this deprecation.

Updated app-checking to allow for linked apps

92099af

millerdev approved these changes Apr 23, 2024

View reviewed changes

orangejenny added 3 commits April 23, 2024 15:36

Merge branch 'master' into jls/web-apps-specific-permissions

af278ce

Revert "Updated app-checking to allow for linked apps"

2dc5527

This reverts commit 92099af.

Updated linked domain update to replace app ids with linked app ids

757c2cb

orangejenny requested a review from gherceg as a code owner April 24, 2024 15:45

millerdev approved these changes Apr 24, 2024

View reviewed changes

esoergel approved these changes Apr 24, 2024

View reviewed changes

orangejenny added QA Passed and removed awaiting QA QA in progress. Do not merge labels Apr 29, 2024

orangejenny merged commit 7bbfe26 into master Apr 30, 2024
12 of 13 checks passed

orangejenny deleted the jls/web-apps-specific-permissions branch April 30, 2024 13:43

This was referenced May 1, 2024

Don't check web apps group-based permissions unless flag is on #34538

Merged

Added script to grant web apps access to default mobile worker roles #34544

Open

Extract function to retrieve web apps available to user into utility #34546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support app-specific web apps permissions for web apps users #34421

Support app-specific web apps permissions for web apps users #34421

orangejenny commented Apr 11, 2024 •

edited

esoergel left a comment

esoergel Apr 11, 2024

esoergel Apr 11, 2024

esoergel Apr 11, 2024

esoergel Apr 11, 2024

esoergel Apr 11, 2024

orangejenny Apr 11, 2024

esoergel Apr 11, 2024

orangejenny Apr 11, 2024

esoergel Apr 11, 2024

esoergel Apr 11, 2024

orangejenny Apr 11, 2024

esoergel Apr 11, 2024

orangejenny Apr 11, 2024

esoergel Apr 12, 2024 •

edited

esoergel Apr 11, 2024

orangejenny Apr 11, 2024

orangejenny Apr 18, 2024

snopoke left a comment

MartinRiese commented Apr 12, 2024

Robert-Costello commented Apr 12, 2024

orangejenny commented Apr 18, 2024

millerdev Apr 18, 2024

orangejenny Apr 18, 2024

esoergel commented Apr 18, 2024 •

edited

esoergel commented Apr 18, 2024

orangejenny commented Apr 22, 2024

millerdev Apr 24, 2024

orangejenny Apr 24, 2024

esoergel left a comment

esoergel Apr 24, 2024

esoergel Apr 24, 2024

orangejenny Apr 24, 2024

		for access in ApplicationAccess.objects.filter(restrict=True):
		WEB_APPS_PERMISSIONS_VIA_GROUPS.set(access.domain, enabled=True, namespace=NAMESPACE_DOMAIN)



		@skip_on_fresh_install
		def _update_default_mobile_worker_role(apps, schema_editor):

		role = UserRole.objects.get_by_domain(self.downstream_domain)[0]
		self.assertListEqual(role.permissions.web_apps_list, [downstream_app.id])

Support app-specific web apps permissions for web apps users #34421

Support app-specific web apps permissions for web apps users #34421

Conversation

orangejenny commented Apr 11, 2024 • edited

Product Description

Overview

Old behavior

Impetus for change

New behavior

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

esoergel left a 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

esoergel Apr 12, 2024 • edited

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

snopoke left a comment

Choose a reason for hiding this comment

MartinRiese commented Apr 12, 2024

Robert-Costello commented Apr 12, 2024

orangejenny commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel commented Apr 18, 2024 • edited

esoergel commented Apr 18, 2024

orangejenny commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel left a 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

orangejenny commented Apr 11, 2024 •

edited

esoergel Apr 12, 2024 •

edited

esoergel commented Apr 18, 2024 •

edited