EDFIAL-527-update-tenantOwnership guard by rtavernaea · Pull Request #108 · edanalytics/runway

rtavernaea · 2026-07-21T20:06:55Z

I have this in draft right now since there were a couple approaches we discussed but this is what makes the most sense to me at the moment.

Adds isDescendant method to tenant dto
- For now, this can just say if a tenant is a descendant of a global tenant but there is a todo to expand this to all parent/child tenant relationships once we have the full metatenancy hierarchy synced
Adds new app role SupportUser which grants the new privilege job.metatenant.read. I want to confirm this is the only thing we need added to UM before reaching out to Bjorn (tracking in this ticket)
Updates TenantOwnershipGuard
- Converted to a factory function like makeEarthbeamJWTGuard in order to look up the resource's tenant
- The existing logic is now encapsulated in isExactTenantMatch
- The guard expands access for people with access to a job via global tenant.
- I don't know if referencing resourceKey === 'job' is something we want to avoid doing. If so, an alternative approach could be to make the guard only check partner Id and then handle the other checks within the /:jobId route, but that would mean we also need to add the privileges etc for odsConfigs now

amazon-inspector-ohio · 2026-07-21T20:06:59Z

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

snyk-io-us · 2026-07-21T20:07:05Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

amazon-inspector-ohio · 2026-07-21T20:07:41Z

✅ I finished the code review, and didn't find any security or code quality issues.

edandylytics

Hey Rachel -- thanks for putting this together and calling out the big questions in your write-up in the PR description. I kept the review fairly high level, just looking at the overall factoring and things that felt like they'd impact scope.

The last comment might be the place to start first. I think we can use Reflector and a route decorator to remove knowledge of any specific resource or privilege from the guard, and also get a setup that's default-closed to meta-tenant access, so we can just add privileges to the routes we explicitly want to give metatenant access to.

As always, let me know if you'd like to pair on any of this.

edandylytics · 2026-07-23T16:23:02Z

    ])
  ),
  User: Object.freeze(new Set<PrivilegeKey>(['school-year-config.read'])),
+  SupportUser: Object.freeze(new Set<PrivilegeKey>(['job.metatenant.read'])),


SupportUser will also need all privileges that User has.

I know reading Jobs is our focus, let's think about how a few other things would work:

Is there a reason not to do OdsConfigs as well? I think that'd be fine

What about update and create? I think update makes sense for SupportUser but not create. With update there's already an owning tenant on the resource that can be evaluated against the meta-tenant hierarchy, but now so with create.

Currently, there's no update operation for Jobs, but we'll likely add a "resubmit" eventually and that'd be nice to have available.

For now, let's include ODS configs, but keep the privileges scoped to just reading. There are probably some extra wrinkles around updating that we want to consider (e.g. displaying the resource's owning tenant clearly in the UI first so we don't have mistaken edits.)

Should I make any of those front end changes now as part of this PR? If the SupportUser role is enabled for people before including the update privilege for ods configs, we'd want a read only view of the ods config form I assume. But maybe you are imagining we would add that privilege too before anyone actually has this access level?

Good question on the read-only form -- let's actually leave ODS configs out of this PR. It'd be weird UX to have an editable form when the backend will reject updates. And practically, single ODS configs are (a) harder to navigate to directly since we don't link to them and (b) not really that useful for troubleshooting. So ignore that part of my earlier comment and let's just stick to jobs.

edandylytics · 2026-07-23T16:57:25Z

-  constructor(private readonly resourceKey: keyof Request) {
-    this.reflector = new Reflector();
-  }
+export function makeTenantOwnershipGuard(resourceKey: keyof Request) {


I think the original pattern I set up way back when set us on the path for needing a factory function here, but I think we can refactor to simplify how this guard is instantiated.

Parameterizing resourceKey (originally on the constructor, now in the factory fn) is what makes it so that we can't just use normal Nest DI to inject the Prisma client (I believe). But I think we can remove that parameterization. Instead, we can use Reflector to pass the resource from the controller to the guard.

So on the controller, we'd have something like this:

@ResourceKey('job') @UseGuard(TenantOwnershipGuard) class JobController {...}

And then in the Guard, we can just look up the resource key from reflector:

const resourceKey = this.reflector.get<keyof Request>(RESOURCE_KEY, context.getHandler())

We'd have to create ResourceKey, of course. I can't say I love that, but it is more consistent with Nest's approach.

edandylytics · 2026-07-23T16:58:58Z

+      // either the session tenant code and resource tenant code must match exactly, or the
+      // resource tenant must be a descendant of the session tenant AND the user must hold the
+      // job.metatenant.read privilege
+      const isExactTenantMatch =


Most requests will be for exact tenant matches. Let's check that first and allow the request through if it matches. Then we only need to do the DB lookup for the meta-tenancy check in the small minority of cases where it's relevant.

edandylytics · 2026-07-23T17:03:40Z

+      }
+      const resourceTenant = toGetTenantDto(resourceTenantRow);
+      const sessionData = plainToInstance(GetSessionDataDto, request.user);
+      const hasMetatenantJobReadPrivilege = sessionData.privileges.has('job.metatenant.read');


Let's refactor away from checking explicit privileges in this guard. The main reason is that the guard applies to the entire controller but the privileges are specific to each controller method.

One way to do this would be to use Reflector to tag routes with a privilege required for meta-tenant access:

class JobControlls { @AllowMetatenant('job,metatenant.read') get() {...} }

Then in the guard we can check (after doing the exact tenancy check):

is there a privilege that grants meta-tenant access? If not -> reject

does the user have that privilege? If not -> reject

is the owning tenant for the resource a descendant of the user's session tenant? if not -> reject

edandylytics

The updates to the TenantOwnershipGuard look good!

The main thing remaining is test coverage. Currently, we're just exercising "user has privilege" vs "user does not have privilege". But since the privilege operates differently depending on whether the session tenant is global or not, whether the resource tenant is global or not, whether the route allows metatenant access, we should test those dimensions, too.

Most of the remaining comments are fairly minor readability tidying. Overall, this PR is looking good.

edandylytics · 2026-07-29T19:00:58Z

 import { Tenant } from '@prisma/client';
 import { DtoGetBase } from '../utils';

+export const isDescendant = (potentialParent: Tenant, potentialChild: Tenant) => {


Let's move this to the tenant guard file, or maybe a helper file next to the the guard file. That'll keep it in the package it's used in. I wouldn't expect this check needs to be available on the frontend

edandylytics · 2026-07-29T19:28:35Z

      context.getHandler()
    );
-
+    const allowMetatenantPrivilege = this.reflector.get<PrivilegeKey | null>(


For readability, let's set allowMetatenantPrivilege closer to where it's used. Currently, it's squating between setting skipTenantOwnershipCheck and the logic that evaluates skipTenantOwnershipCheck to determine whether to continue.

Ditto for the resourceKey.

edandylytics · 2026-07-29T19:43:30Z

+    if (!resourceTenant) {
+      throw new ForbiddenException('Forbidden'); // resource points at a tenant that doesn't exist
+    }
+    const resourceTenantIsDescendantOfSessionTenant = isDescendant(sessionTenant, resourceTenant);


I'm realizing I have to reference the isDescendent implementation to confirm that the first tenant is intended to be the potential parent and the second the potential child. Rather than positional params, perhaps some named keys would make it easier to read and harder to mix up the order

edandylytics · 2026-07-30T16:45:39Z

Let's also add test cases for all combinations of:

user has / does not have job.metatenant.read

is logged into global tenant / non-global tenant

is attempting to access resource owned by a different global tenant / a non global tenant

Additionally, let's test that a route without the AllowMetatenant decorator does not permit meta-tenant access.

edandylytics · 2026-07-30T16:46:51Z


+// Global tenant for partner A — support users (PartnerAdmin) logged in here
+// get access to any non-global tenant under partner A (tenantA, tenantB).
+export const tenantAGlobal: WithoutAudit<Tenant> = {


could we give it a different letter? E.g. tenantDGlobal? It's unclear reading the tests whether tenantA and tenantAGlobal refer to the same tenant in different states or different tenants altogether.

edandylytics

Updates look good and test well!

There are a couple small logging updates that'd help diagnose cases where we'd issue 500s, but no need for me to re-review after that. Marking approve!

edandylytics · 2026-07-31T14:25:53Z

+    );
+
+    const resource = request[resourceKey];
+        if (!resource || !resourceKey) {throw new InternalServerErrorException()}


Let's add some logging here to indicate why the failure happen.

edandylytics · 2026-07-31T14:27:56Z

+      where: { code_partnerId: { code: resource.tenantCode, partnerId: resource.partnerId } },
+    });
+    if (!resourceTenant) {
+      throw new ForbiddenException('Forbidden'); // resource points at a tenant that doesn't exist


Let's make this a 500, too. A missing tenant ownership is an unexpected state on the server. And logging for visibility would be helpful.

rtavernaea added 3 commits July 21, 2026 12:54

add method

0f91c80

refactor guard

7031702

add tests

4ae2cab

rtavernaea changed the title ~~Edfial 527~~ EDFIAL-527-update-tenantOwnership guard Jul 21, 2026

rtavernaea added 3 commits July 21, 2026 15:16

add todo

33e2e85

apply to jobs only

630f77c

add new privilege

b71a91c

rtavernaea requested a review from edandylytics July 22, 2026 14:20

edandylytics requested changes Jul 23, 2026

View reviewed changes

rtavernaea added 8 commits July 24, 2026 07:47

add decorator

8adee2b

move method

0dc6925

remove resourcekey check

49ca49b

add decorator to other job get routes

1e4db7c

add odsconfig and update privileges

be9960d

add odsconfig and update privileges

7c8612e

remove ods privileges

9721f59

uncomment out privilege check

84926a7

rtavernaea requested a review from edandylytics July 24, 2026 18:09

dont use dto in function

9661ac7

edandylytics requested changes Jul 30, 2026

View reviewed changes

rtavernaea added 2 commits July 30, 2026 12:24

pr comments

79e7252

tests

2e16609

rtavernaea marked this pull request as ready for review July 30, 2026 19:35

rtavernaea requested a review from edandylytics July 30, 2026 19:35

edandylytics approved these changes Jul 31, 2026

View reviewed changes

logging

ee55d10

Uh oh!

Conversation

rtavernaea commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amazon-inspector-ohio Bot commented Jul 21, 2026

Uh oh!

snyk-io-us Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

amazon-inspector-ohio Bot commented Jul 21, 2026

Uh oh!

edandylytics left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edandylytics left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edandylytics left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtavernaea commented Jul 21, 2026 •

edited

Loading

snyk-io-us Bot commented Jul 21, 2026 •

edited

Loading