feat: authorization dataset tiles#2450
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
canViewBlocksemantics for undefined vs emptyauthorization(defaulting to#allbut treating[]as deny-all) are non-obvious; consider making this behavior explicit in the type/usage or using clearer sentinel values (e.g.['#none']) to reduce configuration mistakes. - Filtering
sortedDatasetViewin the component viadatasetView$on everyuserGroups$emission couples authorization logic tightly to the UI; consider moving this filtering into a selector or a dedicated service to keep the component lean and make the logic easier to reuse and test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `canViewBlock` semantics for undefined vs empty `authorization` (defaulting to `#all` but treating `[]` as deny-all) are non-obvious; consider making this behavior explicit in the type/usage or using clearer sentinel values (e.g. `['#none']`) to reduce configuration mistakes.
- Filtering `sortedDatasetView` in the component via `datasetView$` on every `userGroups$` emission couples authorization logic tightly to the UI; consider moving this filtering into a selector or a dedicated service to keep the component lean and make the logic easier to reuse and test.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a2e8a12 to
c91d271
Compare
Junjiequan
left a comment
There was a problem hiding this comment.
I left few comments, would to hear you back what you think
| @@ -83,7 +95,9 @@ export class DatasetDetailDynamicComponent implements OnInit, OnDestroy { | |||
| private router: Router, | |||
| private route: ActivatedRoute, | |||
| private snackBar: MatSnackBar, | |||
| ) {} | |||
| ) { | |||
| this.userGroups$ = this.store.select(selectProfileAccessGroups); | |||
| } | |||
There was a problem hiding this comment.
This could be removed by adding userGroups$ = this.store.select(selectProfileAccessGroups); on line 78
| "--theme-header-5-default": "#7f7f7f", | ||
| "--theme-header-5-default-contrast": "#ffffff", | ||
| "--theme-header-5-lighter": "#e5e5e5", | ||
| "--theme-header-5-lighter-contrast": "#000000", | ||
| "--theme-header-5-darker": "#4c4c4c", | ||
| "--theme-header-5-darker-contrast": "#ffffff", |
There was a problem hiding this comment.
Backend needs to be updated accordingly
|
|
||
| if (auth.length === 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is this condition intentional? that authorization: [] blocks all users from accessing the widget? In the admin panel, if we add an authorization group and then delete it, the frontend config JSON ends up with authorization: [], and nobody can access the widget, until it is changed to authorization: [#all].
I'm not sure which behavior is more intuitive
if (auth.length === 0 || auth.includes("#all")) {
return true;
}or return false when authorization is an empty array
There was a problem hiding this comment.
Yes, this seems inconsistent, arising from the fact that an empty array is "trueish" in js ( as opposed to e.g python), casing line +153 to pass on the empty array. I would adjust the code either map empty to "#all" or if possible handle it in the config setting to set it to undefined/null if all elements are dropped
c91d271 to
3d03bee
Compare
Description
Adds tile-level authorization to dataset detail page for fine-grained access control based on user group memberships.
Motivation
Enables administrators to control which user groups can view specific blocks/tiles in dataset details. Allows hiding s
ensitive metadata, creating group-specific views, and complying with data governance policies while maintaining backwa
rd compatibility.
Fixes:
authorizationfield in admin dashboard JSONForms schema from root level to individual customization itemsChanges:
authorization?: string[]toCustomizationIteminterfaceDatasetDetailDynamicComponentto filter tiles based on user access groups withcanViewBlock()methodheader-5color theme for consistent tile header stylingdatasetViewfrom static array to Observable reacting to user group changesTests included
Documentation
docs/dataset-detail-block-authorization.mdofficial documentation info
Documentation file:
docs/dataset-detail-block-authorization.md(812 lines, comprehensive)Backend version
AI contribution
Summary by Sourcery
Introduce tile-level authorization for dataset detail tiles based on user access groups and update styling to use a new shared tile header theme.
New Features:
Enhancements:
Documentation: