[Workspace] add validation for data source in get and bulk_get methods#7596
Conversation
Signed-off-by: tygao <tygao@amazon.com>
raintygao
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
bandinib-amzn,
curq,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7596 +/- ##
=======================================
Coverage 63.64% 63.65%
=======================================
Files 3634 3634
Lines 80099 80112 +13
Branches 12682 12687 +5
=======================================
+ Hits 50977 50992 +15
+ Misses 26015 26014 -1
+ Partials 3107 3106 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| request: OpenSearchDashboardsRequest | ||
| ) => { | ||
| const requestWorkspaceId = getWorkspaceState(request).requestWorkspaceId; | ||
| if ( |
There was a problem hiding this comment.
Nit: Shall we rewrite to below code?
return !requestWorkspaceId || !!object.workspaces?.includes(requestWorkspaceId)
This looks more clearable to me.
There was a problem hiding this comment.
Do you mean we use this code to replace requestWorkspaceId && !(object.workspaces && object.workspaces?.includes(requestWorkspaceId))? I suppose their effect would not be the same.
There was a problem hiding this comment.
Not replace the if condition. We can replace the whole if statement.
if (
requestWorkspaceId &&
!(object.workspaces && object.workspaces?.includes(requestWorkspaceId))
) {
return false;
} else {
return true;
}can be refactor to
return !requestWorkspaceId || !!object.workspaces?.includes(requestWorkspaceId)It has better readability to me. The method will return true when request workspace id is empty or object's workspaces attributes includes request workspace id.
There was a problem hiding this comment.
!! is used for converting undefined to false, right? This looks more clear, thanks.
There was a problem hiding this comment.
Not a big concern, but I do like your original implementation, it's more readable to me, when I read the code, I can easily get that we only disallow the access to data source when request with workspace but the data source is not associated with the workspace. ;D
Signed-off-by: tygao <tygao@amazon.com>
| return hasPermission; | ||
| } | ||
|
|
||
| // Data source is a workspace level object, validate if data source has the permission to the given `workspaceIds` |
There was a problem hiding this comment.
validate if data source has the permission to the given `workspaceIds`
what do you mean by this? should it be
validate if the request has access to the data source within the requested workspace?
| request: OpenSearchDashboardsRequest | ||
| ) => { | ||
| const requestWorkspaceId = getWorkspaceState(request).requestWorkspaceId; | ||
| return !requestWorkspaceId || !!object.workspaces?.includes(requestWorkspaceId); |
There was a problem hiding this comment.
!requestWorkspaceId this covers the case when outside of a workspace?
There was a problem hiding this comment.
No, in WorkspaceState type, requestWorkspaceId could be undefined. Here just want to ensure that the workspaceId to be compared is existing.
There was a problem hiding this comment.
The requestWorkspaceId should be exists if user in a workspace. In another words, user should be out of workspace if requestWorkspaceId is undefined right?
There was a problem hiding this comment.
We don't need to care whether is in or out of workspace especially here.
There was a problem hiding this comment.
Imaging user is in global(requestWorkspaceId is undefined) and try to get a datasource(without workspaces) by id, we should prevent user from getting the datasource detail.
I think the !requestWorkspaceId should be removed.
There was a problem hiding this comment.
I think this may be a common case in OSD, this doesn't belong to the concept of workspace/assign/unassign, we should not do extra limitation.
There was a problem hiding this comment.
Yeah, agreed. There may be some use cases that users may access data source detail in global.
There was a problem hiding this comment.
But can we add a unit test to cover the case when requestWorkspaceId is empty?
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
#7596) * add data source validation in get and bulk get Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR #7596 created/updated * update wrapper validate implementation Signed-off-by: tygao <tygao@amazon.com> * update comments Signed-off-by: tygao <tygao@amazon.com> * test: add cases when not in workspace Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 424e241) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
#7596) (#7649) * add data source validation in get and bulk get * Changeset file for PR #7596 created/updated * update wrapper validate implementation * update comments * test: add cases when not in workspace --------- (cherry picked from commit 424e241) Signed-off-by: tygao <tygao@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
5 participants
Description
Add validation for data source in get and bulk get
Screenshot
No UX updates
Testing the changes
In workspace, when getting a data source that doesn't belong to this workspace, this request will be forbidden. Before we didn't do this validation.
Changelog
Check List
yarn test:jestyarn test:jest_integration