Skip to content
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

Always show security screen and shows error page when trying to access forbidden data-source #1964

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented May 17, 2024

Description

When a user with multi-data-source feature enabled tries to access the cluster, the Security tab will be rendered based on whether the user has access to connected OpenSearch cluster, and not on whether they have access to remote cluster. This is an inconsistent behavior. To fix this we will display Security tab always when dataSource.enabled feature flag is set to true.

Changes addressed:

  1. Shows security screen when MDS is enabled.
  2. When a user doesn't have access to said backend cluster, a "Permission error" component will be displayed.
  3. Ensures uniform loading experience across page loads and data-source switches.
  4. Handles malformed dataSource object in the url

Category

New feature

Why these changes are required?

To promote seamless experience for multi-data-source users.

What is the old behavior before changes and new behavior after changes?

Before, admins of remote data-source could not access the security configuration features via dashboard.
With this change, admins will have config screen access to clusters it has access to. If they don't have access, a message along the lines You do not have permission to view the data for <cluster-name>. will be displayed.

Issues Resolved

Testing

  • Manual + Automated

1. Only access to Remote DataSource using admin credentials

  • MDS Disabled
Screen.Recording.2024-05-24.at.10.22.01.PM.mov
  • MDS Enabled
    Note: All refreshes that you seen in this video are manual refreshes to show that correct data-source is set in the url.
Screen.Recording.2024-05-24.at.10.18.41.PM.mov

2. Only access to Local DataSource (also remote data-source using admin credentials)

  • MDS Disabled
Screen.Recording.2024-05-24.at.10.25.47.PM.mov
  • MDS Enabled
Screen.Recording.2024-05-24.at.10.55.59.PM.mov

Currently, the remote data-source connection uses stored credentials to interact with remote data-source.

Check List

  • New functionality includes testing
    - [ ] New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a general question - can you also fix the tests?

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 58.46154% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 70.61%. Comparing base (c41a480) to head (d158733).

Files Patch % Lines
...onfiguration/panels/tenant-list/configure_tab1.tsx 25.00% 10 Missing and 2 partials ⚠️
...apps/configuration/panels/service-account-list.tsx 50.00% 4 Missing and 3 partials ⚠️
public/apps/configuration/panels/user-list.tsx 46.15% 4 Missing and 3 partials ⚠️
public/apps/configuration/app-router.tsx 25.00% 6 Missing ⚠️
public/apps/configuration/panels/role-list.tsx 40.00% 2 Missing and 4 partials ⚠️
.../apps/configuration/panels/auth-view/auth-view.tsx 68.75% 4 Missing and 1 partial ⚠️
...uration/panels/permission-list/permission-list.tsx 69.23% 3 Missing and 1 partial ⚠️
...blic/apps/configuration/access-error-component.tsx 66.66% 1 Missing and 1 partial ⚠️
...ps/configuration/panels/tenant-list/manage_tab.tsx 77.77% 1 Missing and 1 partial ⚠️
...s/configuration/panels/tenant-list/tenant-list.tsx 50.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
+ Coverage   69.91%   70.61%   +0.69%     
==========================================
  Files          96       97       +1     
  Lines        2513     2600      +87     
  Branches      346      387      +41     
==========================================
+ Hits         1757     1836      +79     
+ Misses        673      668       -5     
- Partials       83       96      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DarshitChanpura DarshitChanpura marked this pull request as ready for review May 25, 2024 03:00
Signed-off-by: Darshit Chanpura <[email protected]>
Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, thanks for the thorough testing and videos for the change 👍

@DarshitChanpura
Copy link
Member Author

@cwperks @derek-ho I'm marking conversations as resolved. Please re-open them if any comment requires more discussion.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor notes.

@DarshitChanpura
Copy link
Member Author

@scrawfor99 addressed all comments and resolved conversations that were duplicates. Please resolve or comment on the open items so we can address those and bring them to closure.

@cwperks
Copy link
Member

cwperks commented May 30, 2024

@DarshitChanpura Small nit on design. Can the "You do not have permissions text" be centered in the container?

@cwperks
Copy link
Member

cwperks commented May 30, 2024

The change LGTM otherwise. Thank you @DarshitChanpura!

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Jun 3, 2024

@cwperks The alignment is handled by parent component in this case. AccessErrorComponent uses EuiPageContent, which defaults to center justified. If we want to align it correctly we will have to adjust parent component, since all the wrapper component align the text as left justified.
IMO, it is fine to leave it as is and get the PR merged since making changes to parent component is not feasible.

@DarshitChanpura DarshitChanpura merged commit b3e444f into opensearch-project:main Jun 3, 2024
19 of 20 checks passed
@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Jun 3, 2024
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1964-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b3e444fa6f3af1410c10a937a44cb1b2fe0127cb
# Push it to GitHub
git push --set-upstream origin backport/backport-1964-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1964-to-2.x.

DarshitChanpura added a commit to DarshitChanpura/security-dashboards-plugin that referenced this pull request Jun 3, 2024
derek-ho pushed a commit that referenced this pull request Jun 4, 2024
…trying to access forbidden data-source (#1964) (#1984)

* Always show security screen and shows error page when trying to access forbidden data-source (#1964)

Signed-off-by: Darshit Chanpura <[email protected]>

* Fixes unit tests

Signed-off-by: Darshit Chanpura <[email protected]>

* Removes service account related changes

Signed-off-by: Darshit Chanpura <[email protected]>

---------

Signed-off-by: Darshit Chanpura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Always show security plugin screens when Multiple Datasources is Enabled
4 participants