rebenitez1802 commented on code in PR #45752:
URL: https://github.com/apache/superset/pull/45752#discussion_r4390107403
##########
superset/commands/utils.py:
##########
@@ -123,28 +127,40 @@ def populate_subject_list(
return [user_subject] if user_subject else []
return []
+ # Resolve typed references before applying lockout checks. A user ID must
+ # never fall back to the Subject with the same numeric primary key.
+ for reference in subject_ids:
+ if not isinstance(reference, dict):
+ subject = get_subject(reference)
+ else:
+ resolver = {
+ "user": get_user_subject,
+ "role": get_or_create_role_subject,
+ "group": get_or_create_group_subject,
+ "subject": get_subject,
+ }.get(reference["type"])
+ subject = resolver(reference["id"]) if resolver else None
+ if not subject:
+ raise SubjectsNotFoundValidationError(field_name)
+ subjects.append(subject)
Review Comment:
Non-blocking: naming the same principal twice appends its Subject twice, and
the flush then hits the `dashboard_editors` unique constraint. The PUT returns
an opaque 422 "Dashboard could not be updated." with no field info. Examples:
`[41, {"type": "subject", "id": 41}]`, or a `{"type": "user", ...}` ref for
someone already listed by Subject ID.
`[41, 41]` already failed this way on master. The new docs say bare IDs and
typed refs can be combined, though, and GET returns Subject IDs, so clients
that append a typed ref to an existing list will hit this more often. Skipping
already-resolved subjects makes all of these cases succeed:
```suggestion
if subject not in subjects:
subjects.append(subject)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
