appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
34.19k stars 3.7k forks source link

[BUG] User sign up with POST request to the API #803

Closed harishkotra closed 3 years ago

harishkotra commented 4 years ago

Description

I've just sent a POST request using Postman to with plain text JSON Body and boom! Account is created.

Request URL: https://app.appsmith.com/api/v1/users Request: http://prntscr.com/uokz6z

What could go possibly wrong with this approach?

I can simply write a loop that runs probably a 1000 times or 1000...n times and create users with a simple cURL call and random emails and flood the database.

What are the ways to fix this

Add security to disable API based user creation and also add either a reCAPTCHA or a layer of security to verify if it is an actual user signing up.

Important Details

pc9795 commented 4 years ago

We can implement a Rate limiter in the back-end using the token given to the UserPrincipal 🤔

Nikhil-Nandagopal commented 4 years ago

@pc9795 we recently implemented re-captcha for the editor and re-captcha seems like the best way to do this. Would you consider a simple re-captcha implementation?

pc9795 commented 4 years ago

Recaptcha will protect us from the in-app requests but what about machine clients (like cURL, Postman etc)?

Nikhil-Nandagopal commented 4 years ago

@pc9795 We actually recently learnt that re-captcha is a backend protection mechanism. It sends the backend a token that is generated by the re-captcha library and the token has to be verified by a google API. https://developers.google.com/recaptcha/docs/verify

pc9795 commented 4 years ago

Oh! If we are implementing the re-captcha token validation in the backend then it works fine. I just had seen this on client-side that's why I was confused 👍

pc9795 commented 4 years ago

If we have implemented it then we can close this issue.

Nikhil-Nandagopal commented 4 years ago

@pc9795 it's not implemented yet. Would you like to pick this up?

pc9795 commented 4 years ago

I will have a look 👍

sumitsum commented 3 years ago

Hi @Nikhil-Nandagopal, Can I take it up if it is available ?

monarch0111 commented 3 years ago

Hey @sumitsum, are you working on this issue? If not, can I take this over?

I have gone through the codebase to understand the flow for this issue. I have few questions, if you can help me clarify on to get on speed with this issue.

In UserController there are 2 methods which can be used to create new users.

As per above flow UserSignup#signupAndLogin is the point where both flows converge.

Question: I don't know where UserController#create method is being called from (I guess, it's an API interface for clients other than WebAPP). Where should recaptcha verification step fall in?

If we only want to add recaptcha for signup flow, I feel UserSignup#signupAndLogin should be a good point. But, it voilates Single Responsibility Priciple of method.

Ideally, a middleware (using annotation) should be used to keep it extensible for all methods in future (i.e. login, password change etc).

I am new to Java, please correct me if I get terminologies wrong.

sharat87 commented 3 years ago

Hey @monarch0111, you're on the right track. Only change I'd suggest is instead of an annotation, let's just have a service class called CaptchaValidatorService or something that has a method to do the recaptcha validation. Now whatever API endpoints can reach out to this service, if that API needs to be protected by a captcha.

Check out this link: https://www.baeldung.com/spring-security-registration-captcha#Retrieve-ResponseToken. This suggests a way of doing the check in the request handler method and then proceeding to rest of the implementation. It'll be slightly different in our case since Appsmith's codebase is reactive in nature, but this should get you started. The rest of this article can also serve as a starting point for an implementation of the validator service.

Do reach out here if you get stuck somewhere or need a pointer in the codebase itself. Good luck 👍 and thanks for your contributions 🙏!

monarch0111 commented 3 years ago

@sharat87 Thanks for the insights. I have done a basic implementation (not tested well, though). https://github.com/monarch0111/appsmith/pull/1

It needs to be refined and worked on, I will raise a PR on this repo once done.

mohanarpit commented 3 years ago

Will there be a way to bypass/mock the recaptcha as well? I'm asking this because a few deployments (like AWS AMI) create a default admin user. Also our Cypress tests create test user accounts for each test.

I would like to ensure that there's a way to either bypass or mock re-captcha invocations to ensure that we don't end up breaking any of these tests or self-hosted instances.

monarch0111 commented 3 years ago

@mohanarpit I have defined an interface, which should allow us to mock this. e.g. https://github.com/monarch0111/appsmith/pull/1/files#diff-249dac13c37b04dc4c59b32ecb1231dc35862b0c5de50ecb6f7919351ee7f4c0R51

In case secret key is not defined, it will always return true as per current implementation. I will move it to a flag based configuration to enable/disable captcha verification, so that it's explicit.