-
Notifications
You must be signed in to change notification settings - Fork 208
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): Add implicit conditions to configuration schema #475
Conversation
Error: Error while trying to collect info after merging O3-1207-implicit-permission-wrapper into master. Error: git merge command failed (is there a merge conflict?) at commentPrImpact (file:///home/runner/work/openmrs-esm-core/openmrs-esm-core/node_modules/@jsenv/github-pull-request-impact/src/commentGitHubPullRequestImpact.js:314:13) at processTicksAndRejections (node:internal/process/task_queues:96:5) at async commentGitHubPullRequestImpact (file:///home/runner/work/openmrs-esm-core/openmrs-esm-core/node_modules/@jsenv/github-pull-request-impact/src/commentGitHubPullRequestImpact.js:82:12) at async file:///home/runner/work/openmrs-esm-core/openmrs-esm-core/tools/size-reporter.mjs:6:1 Generated by @jsenv/file-size-impact during Report bundle size#2694228253 on 780c120 |
packages/framework/esm-api/src/shared-api-objects/current-user.ts
Outdated
Show resolved
Hide resolved
c: 2, | ||
diff: 2, | ||
}, | ||
"Display conditions": { privileges: [] }, |
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.
Why should Display conditions
be available in the config object? Shouldn't it just be handled behind the scenes?
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.
Implementation-wise this seemed simpler than adding it implicitly and then also adding it implicitly to the implementer tools.
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.
I really like your implementation. Maybe we can just delete
that key from the object returned by getConfig
?
/** | ||
* The displayConditionsSchema is implicitly included in every configuration schema | ||
*/ | ||
const displayConditionsSchema: ConfigSchema = { |
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.
This is quite different from how we handle the extension config. But it is a cool idea, and might actually be a better approach than what we currently have implemented. The only tricky part, at first glance, is that IMO these special keys (extensionSlots
, Display conditions
) should not be included in the resolved config object returned by useConfig
/getConfig
.
This is awesome, thanks @ibacher . I'm inclined to call this the implementation of https://issues.openmrs.org/browse/O3-1207 . I think you have improved on the naming, which is great. And your rationale about permissions being additive seems reasonable to me. I think it is very important that the configured value of "permissions" override the one provided in code. It would be great if the implementer tools config object also included the existing extension permissions (the ones from code) as the "default" value. I think we should look into bringing the implementation of |
That's reasonable. I didn't implement this initially just because it means taking a bit of a performance hit, but as a practical matter, though, it's unlikely to make much of a difference. I agree with hiding this config bit when |
@brandones I've implemented the part to hide the display conditions from consuming apps. Getting this to override extension-level privileges settings turns out to require some major refactoring. The underlying problem is that we only track extension configs only for mounted extensions and this means that from the perspective of the code that hides the extensions, the configuration never exists. |
expect(result).toStrictEqual({ | ||
bar: "qux", | ||
baz: "bazzy", | ||
"Display conditions": { privileges: [] }, |
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.
I expect it not to appear here, either
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.
Awesome, thank you @ibacher !
We will eventually have to do the refactoring necessary to make configured permissions override in-code ones.
Requirements
For changes to apps
If applicable
Summary
This adds an implicit configuration section to every configuration schema called
"Display conditions"
1. Currently,"Display conditions"
has only one possible setting:privileges
which is an array of one or more privileges that the user must have to be able to see any extension configured with that configuration schema. It defaults to not restricting. The"Display conditions"
section is settable from anywhere in the configuration system (e.g., slot-specific configuration, extension-in-slot configuration). Note that this also means that no configuration scheme can have a top-level key named"Display conditions"
. The validator does check for this and will log an error if an app tries to create a config schema with a"Display conditions"
keys.Usage:
There are two important limitations here:
privilege
setting that can be returned as part of a page or extension definition. I'm open to discussing changing this."Display conditions"
section only has any effect on extensions.Will ensure that extensions declared in
@openmrs/esm-patient-biometrics-app
only appear if the user has the "View Obs" privilege.Screenshots
Related Issue
Other
This PR also contains one technically breaking change; namely
userHasAccess()
now always returns a boolean. I checked the various repos including OHRI and did not find any uses ofuserHasAccess()
that depended on it's previous return value (which was either the string of the permission checked orundefined
).There are a couple of additional, backwards compatible API enhancements:
userHasAccess()
can now take either a string for a single permission or an array of multiple permissions. If multiple permissions are specified, the user must have all of those permissions.2privilege
that can be returned for pages and extensions insetupOpenMRS()
can now likewise be an array as can theprivilege
property on theUserHasAccess
component.Footnotes
O3-1207 suggested calling this section just
"conditions"
. However,"conditions"
seemed ambiguous—especially since a "condition" has a very different meaning in the context of clinical practice. ↩This decision seems like it might need some justification since it's may not be unreasonable to want to specify multiple privileges and show something if the user has any of the specified privileges. My thinking here is that the most common use-case for privileges in the frontend is to enable hiding widgets from users where the backend will reject the request anyways and permissions on the backend are always additive in this way. ↩