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

Enhance Security by Verifying isSuperAdmin on the Server Side Instead of Using Local Storage #2233

Closed
krishna619 opened this issue Apr 19, 2024 · 6 comments
Labels
feature request unapproved Unapproved for Pull Request

Comments

@krishna619
Copy link

Is your feature request related to a problem? Please describe.
I suggest modifying the superAdminCheck function to validate isSuperAdmin status directly from the server-side fetched profile using the user's ID, rather than relying on client-side stored values.

Describe alternatives you've considered
Someone cannot access the data that the super admin is authorized to access, even if they modify the value of isSuperAdmin.

import { USER_NOT_AUTHORIZED_SUPERADMIN } from "../constants";
import { errors, requestContext } from "../libraries";
import { AppUserProfile } from "../models"; // assuming import of the model

export const superAdminCheck = async (userId: string): Promise<void> => {
  const currentUserAppProfile = await AppUserProfile.findOne({
    userId: userId,
  });

  const userIsSuperAdmin: boolean = currentUserAppProfile?.isSuperAdmin ?? false;

  if (!userIsSuperAdmin) {
    throw new errors.UnauthorizedError(
      requestContext.translate(USER_NOT_AUTHORIZED_SUPERADMIN.MESSAGE),
      USER_NOT_AUTHORIZED_SUPERADMIN.CODE,
      USER_NOT_AUTHORIZED_SUPERADMIN.PARAM,
    );
  }
};

Approach to be followed (optional)
A clear and concise description of approach to be followed.

Additional context
Add any other context or screenshots about the feature request here.

Potential internship candidates
Please read this if you are planning to apply for a Palisadoes Foundation internship PalisadoesFoundation/talawa#359

@github-actions github-actions bot added the unapproved Unapproved for Pull Request label Apr 19, 2024
@pranshugupta54
Copy link
Member

Similar to PalisadoesFoundation/talawa-admin#1839

Can we not use state management to solve this instead of making an API call on every page?
Your approach will make too many API calls.

@krishna619
Copy link
Author

Do you want to use redux for that? yes, we can use but it is a big task in itself.

superAdminCheck fn is used by almost every mutation, for the time being, what I suggest is to change the implementation of the same, so that we are not dependent on just the isSuperAdmin field.

Similar to PalisadoesFoundation/talawa-admin#1839

Can we not use state management to solve this instead of making an API call on every page? Your approach will make too many API calls.

@pranshugupta54
Copy link
Member

I know it's a big task but we have too many flaws for the authentication, authorization so it should all be done together.
We shouldn't add superAdmin check for each page and then make other queries, thats gonna make double queries.

@krishna619
Copy link
Author

We need to discuss with other contributors about this. Also, an auth library is necessary for session management, RBAC, security and other purposes.
This topic was discussed up until a few weeks ago.

@pranshugupta54
Copy link
Member

Yes and this is something more important

@palisadoes
Copy link
Contributor

  1. I'm going to close this.
  2. We have a Calico Challenge project for this. If no one is selected for this project, then we can create a project to finally rectify this

@palisadoes palisadoes closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request unapproved Unapproved for Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants