-
Notifications
You must be signed in to change notification settings - Fork 0
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: accomplish dashboard_admin #95
feat: accomplish dashboard_admin #95
Conversation
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Codecov Report
@@ Coverage Diff @@
## workspace #95 +/- ##
==========================================
Coverage 65.78% 65.78%
==========================================
Files 3334 3334
Lines 64441 64404 -37
Branches 10258 10248 -10
==========================================
- Hits 42390 42370 -20
+ Misses 19482 19463 -19
- Partials 2569 2571 +2
Flags with carried forward coverage won't be shown. Click here to find out more. see 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -140,6 +144,14 @@ export class WorkspaceSavedObjectsClientWrapper { | |||
} | |||
} | |||
|
|||
private isDashboardAdmin(request: OpenSearchDashboardsRequest): boolean { | |||
const config = this.config || ({} as 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.
Looking at this piece of code, it seems there is a possibility that when isDashboardAdmin
is called, but this.config
hasn't initialized, in that case, isDashboardAdmin
will return false which causes the client wrapper to return incorrect client functions if the current user is admin. Correct me if I'm wrong :)
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 figured it out, the isDashboardAdmin
check is run on each request, then I think it will be fine.
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.
Yeah, the wrapper will be called in every request(though I think it is a waste of computing capability) so we can return back the right saved objects client according to backend roles of current requesting user.
* feat: accomplish dashboard_admin Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add yml default config and comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: accomplish dashboard_admin Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add yml default config and comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: accomplish dashboard_admin Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add yml default config and comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Description
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr