-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Working implementation for policies #11
Conversation
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! This is an important addition :)
I left quite some comments. Some can probably be moved to next PRs (please open issues for this). Otherwise might be good to address now
packages/core/src/resolveTrustChains/policies/applyMetadataPolicyToMetadata.ts
Show resolved
Hide resolved
// With one_of it's allowed to not have a value and can be enforced with essential | ||
if (targetValue === undefined) break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we break in this case? Or continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait i was confused. The for loop and switch means the break only applies to the switch. In this case I'd go for using if/else to make it easier to detect where the break applies.
Because in this case the break means basically the same as continue I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait i was confused. The for loop and switch means the break only applies to the switch. In this case I'd go for using if/else to make it easier to detect where the break applies.
Currently everything is like a early return for the switch so it breaks or it throws.
So what do you mean exactly? Get rid of the switch or the if early return inside the switch?
Because in this case the break means basically the same as continue I think?
Yes in this case it doesn't really mind if it's a continue or break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of the switch
Yeah get rid of the switch. To me it's very confusing if you have a for loop and a switch nested, as it's not immediately clear what a continue or break mean (the break applies to the switch, but the for loop also can have a break)
It's ok for now (it's mostly a personal preference), but I'd write it as:
export async function applyMetadataPolicyToMetadata({
leafMetadata,
policyMetadata,
}: { leafMetadata: Metadata; policyMetadata: Record<string, Record<string, MetadataPolicyOperator>> }) {
const resolvedLeafMetadata = new MetadataHelper(cloneDeep(leafMetadata))
for (const [serviceKey, service] of objectToEntries(policyMetadata)) {
for (const [servicePropertyKey, policyValue] of objectToEntries(service)) {
const policies = objectToEntries(policyValue)
.filter(([key]) => isExistingPolicyKey(key))
.sort(
([policyKeyA], [policyKeyB]) =>
allSupportedPolicies[policyKeyA as SupportedPolicyKey].orderOfApplication -
allSupportedPolicies[policyKeyB as SupportedPolicyKey].orderOfApplication
)
const path = `${serviceKey}.${servicePropertyKey}`
for (const [policyPropertyKey, valueFromPolicy] of policies) {
if (policyPropertyKey === 'value') {
resolvedLeafMetadata.setPropertyValue(serviceKey, servicePropertyKey, valueFromPolicy)
} else if (policyPropertyKey === 'add') {
const targetValue = resolvedLeafMetadata.getPropertyValue<PolicyValue>(serviceKey, servicePropertyKey) ?? []
if (!Array.isArray(targetValue))
throw new PolicyValidationError('Cannot apply add policy because the target is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
const newValue = union(targetValue, valueFromPolicy)
resolvedLeafMetadata.setPropertyValue(serviceKey, servicePropertyKey, newValue)
} else if (policyPropertyKey === 'one_of') {
const targetValue = resolvedLeafMetadata.getPropertyValue<PolicyValue>(serviceKey, servicePropertyKey)
// With one_of it's allowed to not have a value and can be enforced with essential
if (targetValue === undefined) continue
if (!Array.isArray(valueFromPolicy))
throw new PolicyValidationError('Cannot apply one_of policy because the value is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
if (!valueFromPolicy.some((value) => value === targetValue))
throw new PolicyValidationError('Cannot apply one_of policy because the intersection is empty', {
path,
policyValue: valueFromPolicy,
targetValue,
})
}
else if (policyPropertyKey === 'default') {
if (resolvedLeafMetadata.hasProperty(serviceKey, servicePropertyKey)) continue
resolvedLeafMetadata.setPropertyValue(serviceKey, servicePropertyKey, valueFromPolicy)
}
else if (policyPropertyKey === 'subset_of') {
const targetValue = resolvedLeafMetadata.getPropertyValue<PolicyValue>(serviceKey, servicePropertyKey)
if (!Array.isArray(targetValue))
throw new PolicyValidationError('Cannot apply subset_of policy because the target is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
if (!Array.isArray(valueFromPolicy))
throw new PolicyValidationError('Cannot apply subset_of policy because the value is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
const newValue = targetValue.filter((value) => valueFromPolicy.includes(value))
if (newValue.length === 0) {
resolvedLeafMetadata.deleteProperty(serviceKey, servicePropertyKey)
} else {
resolvedLeafMetadata.setPropertyValue(serviceKey, servicePropertyKey, newValue)
}
}
else if (policyPropertyKey === 'superset_of') {
const targetValue = resolvedLeafMetadata.getPropertyValue<PolicyValue>(serviceKey, servicePropertyKey)
if (!Array.isArray(targetValue))
throw new PolicyValidationError('Cannot apply superset_of policy because the target is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
if (!Array.isArray(valueFromPolicy))
throw new PolicyValidationError('Cannot apply superset_of policy because the value is not an array', {
path,
policyValue: valueFromPolicy,
targetValue,
})
if (!targetValue.every((value) => valueFromPolicy.includes(value)))
throw new PolicyValidationError('The target does not contain all the values from the policy superset', {
path,
policyValue: valueFromPolicy,
targetValue,
})
}
else if (policyPropertyKey === 'essential') {
if (!valueFromPolicy) continue
const targetValue = resolvedLeafMetadata.getPropertyValue<PolicyValue>(serviceKey, servicePropertyKey)
if (targetValue === undefined)
throw new PolicyValidationError('The policy is required to have a value', {
path,
policyValue: valueFromPolicy,
targetValue,
})
if (Array.isArray(targetValue) && targetValue.length === 0)
throw new PolicyValidationError('The target is empty and is essential to have a value', {
path,
policyValue: valueFromPolicy,
targetValue,
})
}
}
}
}
return {
resolvedLeafMetadata: resolvedLeafMetadata.metadata,
}
}
packages/core/src/resolveTrustChains/policies/applyMetadataPolicyToMetadata.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Tom Lanser <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
🦋 Changeset detectedLatest commit: 2e2d609 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
An implementation for the policies based on the latest openid federation draft 41 spec.
The only thing what it doesn't support currently is having objects in the array for the operators. These were also optional in the spec. I mostly let it out because I wasn't really sure about how we should compare the objects and when they will occur.
I tried to make everything consistent but a lot of things were refactored at the time of implementation. So when you spot some inconsistencies please add a comment.
NOTE: The types for the operators are not resolved correctly in the main schema tried to fix it multiple times but didn't manage to fix it. The validation is correct.