Skip to content

Commit

Permalink
feat(gate)!: policies should affect visibility on introspection (#963)
Browse files Browse the repository at this point in the history
This ended up becoming  a full rewrite of the introspection logic.
For context, the old implementation generated types dynamically but also
did a few parts statically (the __schema.types list). This worked as
long as we assume all object types fields are not context dependent.
Which resulted in a few bugs on injected fields.

In this new implementation, we "define" a type as required (it is added
into the __schema.types list), still recursively but this allows
emitting types with adhoc suffixes in demand depending on the policies
or injections. Then when the type is refered we simply give a reference
to it.

#### Migration notes

* There can be multiple empty object scalars (vs only one previously)
* Fields can be missing if not authorized
* Type names depends on its shape (e.g.Foo missing a field would have
different name than Foo with all its field), this extends to unions
(depends on the variant names with each following the same naming rule)


- [x] The change comes with new or modified tests
- [ ] Hard-to-understand functions have explanatory comments
- [ ] End-user documentation is updated to reflect the change


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **New Features**
- Enhanced GraphQL introspection now offers a more comprehensive and
consistent schema view with refined field visibility controls.
- New introspection queries and fragments improve the introspection
capabilities of the GraphQL API.
- Introduced new functions for managing type visibility and policies
within the type graph context.
- Added new test functions to validate introspection queries and their
structures.

- **Refactor**
- Improved runtime initialization and error handling lead to a smoother
query engine performance and more robust schema generation.
- Code structure has been enhanced for better organization and clarity,
particularly in type visibility management.
- Modifications to authorization checks and policy evaluations enhance
granularity and processing efficiency.

- **Tests**
- Expanded test coverage validates the new introspection enhancements
and policy enforcement, ensuring reliable operation.
- New tests focus on visibility checks and policy rule logic during
introspection on complex and simple type graphs.
- Added tests for introspection functionalities and visibility policies
in various scenarios, ensuring comprehensive validation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
michael-0acf4 authored Feb 6, 2025
1 parent e157897 commit 4227fa0
Show file tree
Hide file tree
Showing 27 changed files with 2,873 additions and 1,386 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
tmate_enabled:
type: boolean
description: |
Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate).
Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate).
This disables all but the test-full jobs.
required: false
default: false
Expand All @@ -20,7 +20,6 @@ on:
- synchronize
- ready_for_review


# Cancel any in-flight jobs for the same PR/branch so there's only one active
# at a time
concurrency:
Expand Down Expand Up @@ -305,6 +304,7 @@ jobs:
tests/runtimes/wasm_reflected/*.ts \
tests/runtimes/python/*.ts \
tests/runtimes/substantial/common.ts \
tests/introspection/common.ts \
tests/e2e/self_deploy/self_deploy.ts \
tests/e2e/published/*.ts \
tests/metagen/typegraphs/metagen.ts \
Expand Down
4 changes: 2 additions & 2 deletions src/typegate/src/engine/computation_engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,12 @@ export class ComputationEngine {
(lens as Array<Record<string, unknown>>).forEach((l, i) => {
// Objects are replaced by empty objects `{}`.
// It will be populated by child compute stages using values in `cache`.
l[field] = withEmptyObjects(res[i]);
(l ?? {})[field] = withEmptyObjects(res[i]);
});

// TODO
this.lenses[stageId] = lens.flatMap((l) =>
batcher([(l as Record<string, unknown>)[field]]) ?? []
batcher([((l ?? {})as Record<string, unknown>)[field]]) ?? []
);
}

Expand Down
83 changes: 47 additions & 36 deletions src/typegate/src/engine/planner/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,31 +203,33 @@ export class OperationPolicies {
} // elif deny => already thrown
}

const res = await this.#checkStageAuthorization(
const resultsPerField = await this.#checkStageAuthorization(
stageId,
activeEffect,
getResolverResult,
);

switch (res.authorized) {
case "ALLOW": {
resolvedPolicyCachePerStage.set(stageId, "ALLOW");
continue;
}
case "PASS": {
resolvedPolicyCachePerStage.set(stageId, "PASS");
continue;
}
default: {
resolvedPolicyCachePerStage.set(stageId, res.authorized);
const policyNames = res.policiesFailed.map((operand) => ({
name: this.tg.policy(operand.index).name,
concernedField: operand.canonFieldName,
}));

throw new BadContext(
this.#getRejectionReason(stageId, activeEffect, policyNames),
);
for (const { fieldStageId, check } of resultsPerField) {
switch (check.authorized) {
case "ALLOW": {
resolvedPolicyCachePerStage.set(fieldStageId, "ALLOW");
continue;
}
case "PASS": {
resolvedPolicyCachePerStage.set(fieldStageId, "PASS");
continue;
}
default: {
resolvedPolicyCachePerStage.set(fieldStageId, check.authorized);
const policyNames = check.policiesFailed.map((operand) => ({
name: this.tg.policy(operand.index).name,
concernedField: operand.canonFieldName,
}));

throw new BadContext(
this.#getRejectionReason(fieldStageId, activeEffect, policyNames),
);
}
}
}
}
Expand All @@ -238,19 +240,19 @@ export class OperationPolicies {
effect: EffectType,
policiesData: Array<{ name: string; concernedField: string }>,
): string {
const getPath = (concernedField: string) => {
const getPath = () => {
if (stageId == EXPOSE_STAGE_ID) {
return [EXPOSE_STAGE_ID, concernedField].join(".");
return EXPOSE_STAGE_ID;
}
return [EXPOSE_STAGE_ID, stageId, concernedField].join(".");
return [EXPOSE_STAGE_ID, stageId].join(".");
};

const detailsPerPolicy = policiesData
.map(({ name, concernedField }) =>
.map(({ name }) =>
[
`policy '${name}'`,
`with effect '${effect}'`,
`at '${getPath(concernedField)}'`,
`at '${getPath()}'`,
].join(" ")
);
return `Authorization failed for ${detailsPerPolicy.join("; ")}`;
Expand Down Expand Up @@ -407,12 +409,16 @@ export class OperationPolicies {
getResolverResult: GetResolverResult,
) {
const selectedFields = this.#findSelectedFields(stageId);

const checkResults = [];
const policiesForStage = this.#stageToPolicies.get(stageId) ?? [];
const policies = [];
for (const { canonFieldName, indices } of policiesForStage) {
const policies = [];

// Note: canonFieldName is the field on the type (but not the alias if any!)
if (!selectedFields.includes(canonFieldName)) {
const selected = selectedFields.find(({ node }) =>
node == canonFieldName
);
if (!selected) {
continue;
}

Expand All @@ -433,25 +439,30 @@ export class OperationPolicies {
policies.push({ canonFieldName, index: actualIndex });
}
}

checkResults.push({
fieldStageId: selected.stageId,
check: await this.#composePolicies(
policies,
effect,
getResolverResult,
),
});
}

return await this.#composePolicies(
policies,
effect,
getResolverResult,
);
return checkResults;
}

#findSelectedFields(targetStageId: string) {
return this.orderedStageMetadata.map(({ stageId, node }) => {
const chunks = stageId.split(".");
const parent = chunks.slice(0, -1).join(".");
if (parent == "" && targetStageId == EXPOSE_STAGE_ID) {
return node;
return { stageId, node };
}

return targetStageId == parent ? node : null;
}).filter((name) => name != null);
return targetStageId == parent ? { stageId, node } : null;
}).filter((val) => val != null);
}

// for testing
Expand Down
Loading

0 comments on commit 4227fa0

Please sign in to comment.