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

Create Organization Security Bug #1061

Closed
kb-0311 opened this issue Feb 12, 2023 · 6 comments · Fixed by #1065
Closed

Create Organization Security Bug #1061

kb-0311 opened this issue Feb 12, 2023 · 6 comments · Fixed by #1065
Assignees
Labels
bug Something isn't working feature request security Security fix

Comments

@kb-0311
Copy link
Contributor

kb-0311 commented Feb 12, 2023

Describe the bug
A clear and concise description of what the bug is.

In the talawa-admin portal the users needs to be a verified SUPERADMIN to get access to the portal logically. In the Dashboard of talawa-admin Portal , there is a create organization function.

In short a user needs to be a super admin to create an organization according to the logic. However, in the backend no check is provided for the user role before creating an organization , thus enabling any one (USER, ADMIN , SUPERADMIN etc )to create an
organization.

This is a security bug.

This is security by obfuscation where capabilities are not presented to the user, but are still available.

To Reproduce
Steps to reproduce the behavior:

  1. Sign up as a user in graphql playground
  2. get the access token
  3. Create and organization through its mutation
  4. An org is created

Expected behavior
A clear and concise description of what you expected to happen.
Only super admins should be allowed to create new organizations.
Actual behavior
A clear and concise description of how the code performed w.r.t expectations.
Right now anyone can technically create new organizations.
Screenshots
If applicable, add screenshots to help explain your problem.

Additional details
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
//SIgn up as a brand new default user
Screenshot from 2023-02-12 11-39-24
//Logged in as that user
Screenshot from 2023-02-12 11-39-33
// Created a brand new organization as userType:USER
Screenshot from 2023-02-12 11-39-41
Screenshot from 2023-02-12 11-41-19
Screenshot from 2023-02-12 11-41-48

@kb-0311 kb-0311 added the bug Something isn't working label Feb 12, 2023
@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 12, 2023

pls assign this to me

@github-actions github-actions bot added feature request security Security fix unapproved Unapproved for Pull Request labels Feb 12, 2023
@anshgoyalevil
Copy link
Contributor

anshgoyalevil commented Feb 12, 2023

@kb-0311 You may resolve this issue using the roleDirective

Just apply the role directive on the createOrganization mutation like so

type Query {
    hello: String @role(requires: "SUPERADMIN")
}

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 12, 2023

@anshgoyalevil Yes but that will not be enough from a modular development perspective imo.

  1. The roleDirective will eventually be implemented for every query/Mutation that requires it. But as you can see in all typeDefs the role directive is not implemented for any directive that requires it. So while I am fixing security flaws I need to make sure to implement them one by one to not cause breaking change.
  2. There needs to be a thorough approach to how I am fixing security-related bugs consistent with the codebase so that other developers can easily debug and write tests in the future. That is why it is important to add a layer of check in the resolver itself as it has been done for all resolvers till now.

Along with roleDirectives in gql I will also be adding an extra utility superAdminCheck.ts within /utilities just like adminCheck.ts and creatorCheck.ts which currently exists, to add a more modular approach and make this function logically reusable whenever needed.

Also speaking of the roleDirectives, there is a slight mistake with the thrown error->

Screenshot from 2023-02-12 12-53-42

the error should be 'user.notAuthorised'. 'user.notAuthenticated' is an error thrown by auth directive(which could get very confusing to differentiate the cause of error while manually testing and debugging the app) so will be fixing that too in this same PR. and add this as a constant.

@xoldd
Copy link
Contributor

xoldd commented Feb 12, 2023

@kb-0311 You're changing the errors thrown by the server. If the clients check the error for different types to trigger a particular UI behaviour you'll be breaking their functionality. Make sure to remember this.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 12, 2023

@xoldyckk Yes, I will make sure to change the talawa-admin app and talawa-mobile app accordingly.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 12, 2023

@xoldyckk could you pls remove the unapproved label too ?

@xoldd xoldd removed the unapproved Unapproved for Pull Request label Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature request security Security fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants