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

[BUG-1427] Newly created user defaults to same tenant as admin when using same browser window. #1614

Conversation

prabhask5
Copy link
Contributor

@prabhask5 prabhask5 commented Oct 18, 2023

Description

Fixes bug that defaults a new user to the Global tenant, even when they do not have permission for the Global tenant and this option is blocked out on the modal. To fix this, I added a new check to the resolveTenantName function, in which it checks of whether the Global tenant is available for this account through a new parameter to the function, in that case it skips over the Global tenant return and returns a Private tenant.

Category

Bug fix

Why these changes are required?

New users were having the Global tenant role even when they don't have the permissions to, which is a security risk.

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

Old behavior: When you select global tenant on the admin account then switch to a new user, this global tenant is selected by default even when this option is not available since the new user doesn't have access to the Global tenant.
Screenshot 2023-10-18 at 12 40 40 AM

New behavior: In this case, the default option automatically switches to the next accessible role, which is the Private tenant.
Screenshot 2023-10-18 at 12 52 10 AM

Issues Resolved

Fixed issue #1427

Testing

Manually tested to make sure nothing was broken. Additionally added new tests to tenant-utils.test.tsx to update function and test for cases where the global tenant is not enabled.

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.

Signed-off-by: Prabhas Kurapati <[email protected]>
@cwperks
Copy link
Member

cwperks commented Oct 20, 2023

@prabhask5 I have a few general questions.

  1. I think its possible that cluster admins can disable the private tenant. In that case how will this PR handle that scenario?

  2. Is it ever possible that a user has access to 0 tenants? If so, what is the behavior?

  3. If private or global are disabled does this PR resort to the first tenant for which the user has access?

@cwperks
Copy link
Member

cwperks commented Oct 20, 2023

FYI The CI is unstable in this repo atm. There is an open PR to address CI stability: #1613

You may need to rebase after merge of that PR to get more accurate CI results.

@prabhask5
Copy link
Contributor Author

Closing to start over on code changes to consider questions asked in PR comments

@prabhask5 prabhask5 closed this Oct 25, 2023
@prabhask5 prabhask5 deleted the pk-new-user-default-to-same-tenant-bug branch October 25, 2023 08:09
@prabhask5
Copy link
Contributor Author

@prabhask5 I have a few general questions.

  1. I think its possible that cluster admins can disable the private tenant. In that case how will this PR handle that scenario?
  2. Is it ever possible that a user has access to 0 tenants? If so, what is the behavior?
  3. If private or global are disabled does this PR resort to the first tenant for which the user has access?

@cwperks I'm not sure on what conditions the global and private tenants are disabled- does this depend on the data in the accountInfo tenantsList or does it depend on other info (either the config global disabled properties or the readonly roles), or both?

I'm also not sure if there's ever a scenario where there is access to 0 tenants- in my new changes I'm allowing the function to return an empty string for tenant, but when I try to do this on my local build that part doesn't work properly- so I'm assuming this can't happen?

@prabhask5
Copy link
Contributor Author

prabhask5 commented Oct 25, 2023

@prabhask5 I have a few general questions.

  1. I think its possible that cluster admins can disable the private tenant. In that case how will this PR handle that scenario?
  2. Is it ever possible that a user has access to 0 tenants? If so, what is the behavior?
  3. If private or global are disabled does this PR resort to the first tenant for which the user has access?

@cwperks I'm not sure on what conditions the global and private tenants are disabled- does this depend on the data in the accountInfo tenantsList or does it depend on other info (either the config global disabled properties or the readonly roles), or both?

I'm also not sure if there's ever a scenario where there is access to 0 tenants- in my new changes I'm allowing the function to return an empty string for tenant, but when I try to do this on my local build that part doesn't work properly- so I'm assuming this can't happen?

For additional info, currently I'm using the following checks to see if the global and private tenants are disabled respectively:

const isGlobalEnabled = config.multitenancy.tenants.enable_global;
const shouldDisableGlobal = !isGlobalEnabled || !tenants.includes('global_tenant');

const isPrivateEnabled = dashboardsInfo.private_tenant_enabled;
const shouldDisablePrivate = !isPrivateEnabled || !tenants.includes(userName) || readonly;

But this is causing some tests to fail as they don't have the private and global tenants in the accountInfo tenants list so this util function thinks they are disabled when they are not. Am I wrong or are the tests wrong?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants