fix: check authorities in app adapter [LIBS-370] #757

KaiVandivier · 2022-10-17T08:23:39Z

Includes the fixes for LIBS-356 and LIBS-357 (which were split off from here and merged in separate PRs)

Checks authorization on the client side by using an AuthBoundary component to check correct authorities and block the app if necessary. This is needed because some deployment environments don't return a 403 'Forbidden' response (like standalone deployments and Netlify, it seems)

Todo:

Get correct app name (and maybe format) for auth checks from CLI and add to app env
Add UI for "Forbidden" screen instead of just hitting the error boundary
Clean up useVerifyLatestUser

UI Draft, updated:

adapter/src/components/AppWrapper.js

KaiVandivier · 2022-10-17T14:32:18Z

adapter/src/components/AuthBoundary.js

+    // Skip check on dev
+    // TODO: should we check on dev environments too?
+    if (!IS_PRODUCTION_ENV) {
+        return true
+    }


Curious what others think -- most app devs probably have admin privileges, but if they don't have app management authority, it's possible that someone might end up working on an a new app which they don't have authorities for.

Someone might be able to bypass their authorities limitations by running an app locally though?...

Currently I guess we don't do any checks like this

(You can make testing easier by commenting out the return true to run it on dev)

I initially wasn't sold on the idea of always returning true in a development environment, but think I'm coming around to it.

Full reload From SW

Production Server renders no-access page App-shell renders no-access view

Development Dev can access app Dev can access app

This is actually very consistent and I like it.

KaiVandivier · 2022-10-17T14:33:45Z

cli/src/lib/formatAppAuthName.js

+    if (config.coreApp) {
+        // TODO: Verify this formatting - are there any transformations,
+        // or do we trust that it's lower-case and hyphenated?
+        return APP_AUTH_PREFIX + DHIS_WEB + config.name
+    }


Curious about whether this formatting of core app names is correct?

In the user app I am also inferring specific types of authorities based on their naming scheme. This has been in production for about 4 years now, so this suggests to me the naming scheme is consistent enough. You can also review the response of this requests: https://play.dhis2.org/dev/api/40/authorities?fields=id,name&paging=false

We do need to do transformation here, as the "name" in the d2 config is NOT necessarily what's used for the authority (or URL name) - see here. We already replicate this logic in the platform here

Ah I see we're doing that below - but we should also do it for core apps.

Yeah, notice that the formatting is different between those two things you linked, namely around hyphens -- core apps seem to be getting a different treatment? But I didn't find what exactly it is

Added some formatting insurance in this commit

pwa/src/service-worker/service-worker.js

KaiVandivier · 2022-10-17T14:34:50Z

pwa/src/service-worker/service-worker.js

+                    if (response.type === 'opaqueredirect' || !response.ok) {
+                        // It's sending a redirect to the login page,
+                        // or an 'unauthorized'/'forbidden' response.
+                        // Return that to the client
                        return response
                    }


Pass 403 responses to the client in production, instead of returning index.html

KaiVandivier · 2022-10-17T14:35:30Z

adapter/src/components/AuthBoundary.js

+const IS_PRODUCTION_ENV = process.env.NODE_ENV === 'production'
+
+const APP_MANAGER_AUTHORITY = 'M_dhis-web-maintenance-appmanager'
+const REQUIRED_APP_AUTHORITY = process.env.REACT_APP_DHIS2_APP_AUTH_NAME


Added in CLI

KaiVandivier · 2022-10-17T14:47:26Z

You can test the boundary by logging in as a guest user (user guest, pass Guest123), which shouldn't have access to all apps -- also see the 'production env' test above that you can comment out to test it in a dev environment locally

HendrikThePendric

Looks very Kai. Thanks for noticing this had slipped through the cracks and turning this around so quickly. I've left a few comments.

adapter/src/components/AuthBoundary.js

HendrikThePendric · 2022-10-17T15:24:49Z

cli/src/lib/formatAppAuthName.js

+    if (config.coreApp) {
+        // TODO: Verify this formatting - are there any transformations,
+        // or do we trust that it's lower-case and hyphenated?
+        return APP_AUTH_PREFIX + DHIS_WEB + config.name
+    }


In the user app I am also inferring specific types of authorities based on their naming scheme. This has been in production for about 4 years now, so this suggests to me the naming scheme is consistent enough. You can also review the response of this requests: https://play.dhis2.org/dev/api/40/authorities?fields=id,name&paging=false

…dary

amcgee · 2022-10-17T16:11:55Z

@KaiVandivier there is quite a bit of overlap here with #754 by the way

KaiVandivier · 2022-10-17T16:12:50Z

@amcgee I noticed that -- I refactored the app wrapper in my latest commit to work well with the changes that you have here: https://github.com/dhis2/app-platform/pull/754/files#diff-58fcbeed2ec3103f3d186cb172bf2da570e316e008fb614240d6aa0f72df7574

Taking a closer look now though

amcgee

Would be good to not duplicate logic between this and the session expiration PR

amcgee · 2022-10-17T16:15:33Z

cli/src/lib/formatAppAuthName.js

+    if (config.coreApp) {
+        // TODO: Verify this formatting - are there any transformations,
+        // or do we trust that it's lower-case and hyphenated?
+        return APP_AUTH_PREFIX + DHIS_WEB + config.name
+    }


We do need to do transformation here, as the "name" in the d2 config is NOT necessarily what's used for the authority (or URL name) - see here. We already replicate this logic in the platform here

amcgee · 2022-10-17T16:17:27Z

cli/src/lib/formatAppAuthName.js

+    if (config.coreApp) {
+        // TODO: Verify this formatting - are there any transformations,
+        // or do we trust that it's lower-case and hyphenated?
+        return APP_AUTH_PREFIX + DHIS_WEB + config.name
+    }


Ah I see we're doing that below - but we should also do it for core apps.

pwa/src/service-worker/service-worker.js

adapter/src/components/AuthBoundary.js

KaiVandivier · 2022-10-20T15:45:04Z

Also added handling for server versions < 35, as in https://github.com/dhis2/app-platform/blob/fix/pwa-handle-navigations/cli/src/lib/constructAppUrl.js#L8

KaiVandivier · 2023-02-16T14:10:06Z

Drafted while this is on the back-burner

KaiVandivier · 2023-02-23T13:26:09Z

I think concerns have been addressed and this is ready for rereview @amcgee

KaiVandivier added 7 commits October 14, 2022 15:49

fix(sw): return !ok navigation responses instead of hitting precache

ce2938b

fix(sw): don't use SWR on .json and .action requests

b5dcd5f

feat(adapter): check user app authorities before rendering app

858546c

fix(adapter): use proper app authority name

b03b858

feat(adapter): add ui for auth boundary

b882ed0

fix(adapter): production env check

ea3ae21

refactor: clean up verifyLatestUser

7f97ca5

KaiVandivier commented Oct 17, 2022

View reviewed changes

adapter/src/components/AppWrapper.js Show resolved Hide resolved

KaiVandivier commented Oct 17, 2022

View reviewed changes

chore: fix comment

1fe6278

KaiVandivier commented Oct 17, 2022

View reviewed changes

pwa/src/service-worker/service-worker.js Outdated Show resolved Hide resolved

KaiVandivier commented Oct 17, 2022

View reviewed changes

KaiVandivier marked this pull request as ready for review October 17, 2022 14:36

HendrikThePendric reviewed Oct 17, 2022

View reviewed changes

refactor: extract useVerifyLatestUser and wrap just app with authBoun…

768e48d

…dary

amcgee requested changes Oct 17, 2022

View reviewed changes

cooper-joe reviewed Oct 18, 2022

View reviewed changes

adapter/src/components/AuthBoundary.js Outdated Show resolved Hide resolved

fix: notice box text

431fa72

KaiVandivier mentioned this pull request Oct 18, 2022

fix(offline): clear SWR caches betwen users [LIBS-358] dhis2/app-runtime#1268

Merged

chore: i18n

7fb97ef

KaiVandivier requested review from amcgee and HendrikThePendric October 19, 2022 08:03

KaiVandivier added 2 commits October 20, 2022 17:38

chore: add name and title to pwa example app

6ddc7e1

fix: ensure core app name formatting and handle legacy versions

09b100c

test: fix tests

0668608

KaiVandivier mentioned this pull request Oct 21, 2022

fix(pwa): file SWR filter & allow navigation 403s [LIBS-356] [LIBS-357] #762

Merged

KaiVandivier marked this pull request as draft February 16, 2023 14:08

KaiVandivier changed the title ~~fix(pwa): handle navigations [LIBS-356] [LIBS-357]~~ fix: check authorities in app adapter [LIBS-356] [LIBS-357] Feb 16, 2023

KaiVandivier changed the title ~~fix: check authorities in app adapter [LIBS-356] [LIBS-357]~~ fix: check authorities in app adapter [LIBS-370] Feb 23, 2023

Merge branch 'master' into fix/pwa-handle-navigations

d5ed304

KaiVandivier marked this pull request as ready for review February 23, 2023 13:25

KaiVandivier removed the request for review from HendrikThePendric February 23, 2023 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check authorities in app adapter [LIBS-370] #757

fix: check authorities in app adapter [LIBS-370] #757

KaiVandivier commented Oct 17, 2022 •

edited by jira bot

KaiVandivier Oct 17, 2022

KaiVandivier Oct 17, 2022

HendrikThePendric Oct 17, 2022 •

edited

KaiVandivier Oct 17, 2022 •

edited

HendrikThePendric Oct 17, 2022

amcgee Oct 17, 2022

amcgee Oct 17, 2022

KaiVandivier Oct 17, 2022 •

edited

KaiVandivier Oct 20, 2022 •

edited

KaiVandivier Oct 17, 2022

KaiVandivier Oct 17, 2022

KaiVandivier commented Oct 17, 2022

HendrikThePendric left a comment

HendrikThePendric Oct 17, 2022

amcgee commented Oct 17, 2022

KaiVandivier commented Oct 17, 2022 •

edited

amcgee left a comment

amcgee Oct 17, 2022

amcgee Oct 17, 2022

KaiVandivier commented Oct 20, 2022

KaiVandivier commented Feb 16, 2023

KaiVandivier commented Feb 23, 2023

	Full reload	From SW
Production	Server renders no-access page	App-shell renders no-access view
Development	Dev can access app	Dev can access app

fix: check authorities in app adapter [LIBS-370] #757

Are you sure you want to change the base?

fix: check authorities in app adapter [LIBS-370] #757

Conversation

KaiVandivier commented Oct 17, 2022 • edited by jira bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HendrikThePendric Oct 17, 2022 • edited

Choose a reason for hiding this comment

KaiVandivier Oct 17, 2022 • 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

KaiVandivier Oct 17, 2022 • edited

Choose a reason for hiding this comment

KaiVandivier Oct 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiVandivier commented Oct 17, 2022

HendrikThePendric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcgee commented Oct 17, 2022

KaiVandivier commented Oct 17, 2022 • edited

amcgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiVandivier commented Oct 20, 2022

KaiVandivier commented Feb 16, 2023

KaiVandivier commented Feb 23, 2023

KaiVandivier commented Oct 17, 2022 •

edited by jira bot

HendrikThePendric Oct 17, 2022 •

edited

KaiVandivier Oct 17, 2022 •

edited

KaiVandivier Oct 17, 2022 •

edited

KaiVandivier Oct 20, 2022 •

edited

KaiVandivier commented Oct 17, 2022 •

edited