rusackas commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3990704803
##########
tests/unit_tests/security/test_permission_instructions_link.py:
##########
@@ -157,6 +209,19 @@ def
test_datasource_error_object_includes_sorted_owner_names() -> None:
error = sm.get_datasource_access_error_object(ds)
assert error.extra is not None
assert error.extra["owners"] == ["Amir Patel", "Zoe Chen"]
+ # The real dataset name must not appear — only a generic placeholder
+ # for backward compatibility with older frontends.
+ assert error.extra["datasource_name"] != "Quarterly Sales"
Review Comment:
bito's right that this only proves the placeholder isn't the real name, not
what it actually is.
```suggestion
assert error.extra["datasource_name"] == "a dataset"
```
##########
superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.test.tsx:
##########
@@ -87,6 +86,32 @@ test('falls back to administrator guidance when no owners
are known', () => {
expect(screen.queryByRole('link', { name: 'Request access' })).toBeNull();
});
+test('treats a pre-is_access_denial payload as an access denial', () => {
+ // During a rolling deploy an older API pod answers with `datasource` and
+ // `datasource_name` and no `is_access_denial` flag. The request-access
+ // guidance must still render, and the dataset name must stay hidden.
+ const props = {
+ ...baseProps,
+ error: {
+ ...baseProps.error,
+ extra: {
+ datasource: 12,
+ datasource_name: 'Quarterly Sales',
+ owners: ['Jane Doe'],
+ },
+ message: 'This endpoint requires the datasource 12',
+ },
+ };
+ render(<DatasourceSecurityAccessErrorMessage {...props} />);
+ expect(
+ screen.getByText("You don't have access to this chart's data"),
+ ).toBeInTheDocument();
+ expect(
+ screen.getByText(/reach out to the chart owner: Jane Doe/),
+ ).toBeInTheDocument();
+ expect(screen.queryByText(/Quarterly Sales/)).toBeNull();
+});
Review Comment:
Nothing here actually proves the link-gating fix stays fixed — this test's
`extra` has no `link`, so there's nothing to suppress. Something like this
would pin it:
```suggestion
});
test('suppresses the request-access link for a pre-is_access_denial
payload', () => {
// A pre-fix backend may have already templated the dataset name into
// `extra.link`. Without `is_access_denial` (or `tables`) to prove the
// payload is safe, the link must not render even though it's present.
const props = {
...baseProps,
error: {
...baseProps.error,
extra: {
datasource: 12,
datasource_name: 'Quarterly Sales',
owners: ['Jane Doe'],
link:
'https://access.example.com/request?dataset=12&name=Quarterly+Sales',
},
message: 'This endpoint requires the datasource 12',
},
};
render(<DatasourceSecurityAccessErrorMessage {...props} />);
expect(screen.queryByRole('link', { name: 'Request access' })).toBeNull();
});
```
--
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]
