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.

Sign up for GitHub

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

Jump to bottom

feat: add recaptcha verification for user signup #3319

Closed
wants to merge 53 commits into from
Closed

feat: add recaptcha verification for user signup #3319

wants to merge 53 commits into from

Conversation

monarch0111
Copy link
Contributor

@monarch0111 monarch0111 commented Mar 2, 2021 •
edited
Loading

Description

  • Services for Google Recaptcha
  1. configurations/GoogleRecaptchaConfig.java
  2. services/GoogleRecaptchaService.java
  3. services/GoogleRecaptchaServiceImpl.java
  • Changes to incoprporate Google Recaptcha Service
  1. controllers/UserController.java
  2. exceptions/AppsmithError.java
  3. solutions/UserSignup.java
  • ENV Variables
  1. resources/application.properties
  2. envs/dev.env.example
  3. envs/docker.env.example

Fixes #803

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  1. Create a new recaptcha config at https://www.google.com/recaptcha/admin/create
  2. Add APPSMITH_RECAPTCHA_ENABLED, APPSMITH_RECAPTCHA_SITE_KEY, APPSMITH_RECAPTCHA_SECRET_KEY` to .env file
  3. Download https://pastebin.com/5G298Y7L & host it locally (e.g ngrok, localhost, over nginx)
  4. Update data-sitekey attribute with site-key obtained in first step & whitelist your domain on admin panel (Note: it doesn't support host+port config)
  5. Start backend server
  6. Click submit

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

TODOs:

  • Add documentation to configure recaptcha verification.
  • Proposal for implementing recaptcha in Frontend.

hetunandu and others added 30 commits January 4, 2021 15:39
@hetunandu
Merge pull request appsmithorg#2416 from appsmithorg/release
a820728 
Release
@hetunandu @ofpiyush
Fix invalid toast message for evaluation error (appsmithorg#2443)
45918bf 
Fixes appsmithorg#2432
Fixes: appsmithorg#2429


Co-authored-by: Piyush Mishra <piyush@codeitout.com>
@trishaanand @sharat87
Increased logs to debug future connection leaks. (appsmithorg#2437)
db2b002 
* Increased logs to debug future connection leaks.

* Fetch a connection from the pool only if a query exists.

* Minor comment added.

* Minor rewrite

* Code formatting.

* Update app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java

Co-authored-by: Shrikant Sharat Kandula <shrikant@appsmith.com>

* Added hikari cp pool stats to get database structure function as well.

Co-authored-by: Shrikant Sharat Kandula <shrikant@appsmith.com>
@trishaanand
Adding action execution time and logging the same (appsmithorg#2473)
d1d7307
@akash-codemonk @hetunandu
Fix onboarding test failure causing other tests to fail (appsmithorg#…
1e17c03 
…2417)
@akash-codemonk @hetunandu
Hide other header elements while showing the deploy button
6e4cdb0
@akash-codemonk @hetunandu
Open the already opened tab if it exists when clicking on deploy (app…
b5a1c93 
…smithorg#2448)
@akash-codemonk @hetunandu
Onboarding fixes (appsmithorg#2458)
44e300e
@akash-codemonk @hetunandu
Onboarding deploy tooltip fix (appsmithorg#2490)
09c4e4f
@sharat87
Merge branch 'release'
63fb249 
# Conflicts:
#	app/client/cypress/integration/Smoke_TestSuite/Onboarding/Onboarding_spec.js
#	app/client/src/components/editorComponents/Onboarding/Tooltip.tsx
#	app/client/src/constants/OnboardingConstants.tsx
#	app/client/src/pages/Editor/Welcome.tsx
#	app/client/src/sagas/ActionExecutionSagas.ts
#	app/client/src/sagas/OnboardingSagas.ts
@hetunandu
Merge branch 'release'
ee985af
@hetunandu
Never show the redux action type in toast errors (appsmithorg#2635)
4014e52
@sharat87
Merge branch 'release'
95250cf 
# Conflicts:
#	app/client/src/constants/messages.ts
#	app/client/src/sagas/ErrorSagas.tsx
@sharat87
Remove duplicate function definition
317fa1e
@sharat87
Merge branch 'release'
1e8aa62
@sharat87
Add Sentry token to enable Sentry integration (appsmithorg#2674)
dc4e741 
(cherry picked from commit b1564c9)
@sharat87
Add message re telemetry during installation (appsmithorg#2621)
3deb106 
(cherry picked from commit 828a9ae)
@mohanarpit
Merge pull request appsmithorg#2759 from appsmithorg/release
433461a 
Release v1.3.2
@sharat87
Merge branch 'release'
779c41e
@sharat87
Merge branch 'release'
ebcbfb0
@Nikhil-Nandagopal
Update UploadingAssets.md
79d1d6d
@sharat87
Merge branch 'release'
95d1717
@sharat87
Merge branch 'release'
464bffa
@sharat87
Fix input error on generating password and salt in install script (ap…
7f58e89 
…psmithorg#3051)

* Fix potential input error on password generation

This happens on macOS systems when a different locale is
explicitly set.

* Don't use a masked variable name

(cherry picked from commit b26706c)
@akash-codemonk @rishabhsaxena @hetunandu
New onboarding flow (appsmithorg#2686)
9c20ac5 
Co-authored-by: a <rishabh.robben@gmail.com>
@akash-codemonk @hetunandu
Go back to application list instead of deleting app (appsmithorg#2981)
e604357
@akash-codemonk @Nikhil-Nandagopal @hetunandu
Update onboarding indicator animation for widget menu (appsmithorg#3018)
86725ba 
Co-authored-by: Nikhil Nandagopal <nikhil@appsmith.com>
@sharat87
Include Firestore document metadata in responses (appsmithorg#3063)
c80b6b4 
(cherry picked from commit b9cb76e)
@sharat87
Push explicitly specified tags to Docker (appsmithorg#2988)
036cb95 
* Push explicitly specified tags to Docker

* Remove now stale comments

(cherry picked from commit 215f1ed)
@sharat87
Add more information for action execution (appsmithorg#3060)
128444f 
* Add more information for action execution

* Add orgId and pageName to action execution data point

(cherry picked from commit 6936a40)
@monarch0111 monarch0111 force-pushed the fix/user-signup-captcha branch from 999b106 to f4916b1 Compare March 2, 2021 12:21
mohanarpit
mohanarpit reviewed Mar 2, 2021
View reviewed changes
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserSignup.java Outdated Show resolved Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/services/GoogleRecaptchaServiceImpl.java Outdated Show resolved Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/services/GoogleRecaptchaServiceImpl.java Outdated Show resolved Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/services/GoogleRecaptchaServiceImpl.java Outdated Show resolved Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/UserController.java Outdated Show resolved Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/services/GoogleRecaptchaService.java Outdated Show resolved Hide resolved
@mohanarpit
Copy link
Member

@monarch0111 Thanks a lot for raising this PR! This was really quick & the code is also good. I've added a few minor comments. Please address them so that we can then merge this change in.

@monarch0111 monarch0111 force-pushed the fix/user-signup-captcha branch 2 times, most recently from 6ad4513 to 214ee9e Compare March 3, 2021 05:55
@monarch0111
Copy link
Contributor Author

monarch0111 commented Mar 3, 2021 •
edited
Loading

@monarch0111 Thanks a lot for raising this PR! This was really quick & the code is also good. I've added a few minor comments. Please address them so that we can then merge this change in.

@mohanarpit Thanks for thorough review. I have made most of the changes, except for isEnabled flag let me know what you think about it. Will makes changes accordingly. Oops! didn't see your comment on that already. 😅

@monarch0111 monarch0111 marked this pull request as ready for review March 3, 2021 06:54
@monarch0111 monarch0111 force-pushed the fix/user-signup-captcha branch from bc5d36d to 67ece23 Compare March 4, 2021 02:00
@monarch0111 monarch0111 closed this Mar 4, 2021
@monarch0111 monarch0111 mentioned this pull request Mar 4, 2021
Merged
9 tasks
@monarch0111
Copy link
Contributor Author

@mohanarpit It had some merge conflict, I rebased it with master instead of release branch. Closed this in favour of #3383

sondermanish pushed a commit that referenced this pull request Mar 6, 2024
@nsarupr
fix: do not show request roles in user group details (#3319)
3b3532e 
## Description
> Do not show Roles associated with Approval requests in User group
details.

#### PR fixes following issue(s)
Fixes [[Bug]: Remove the Approval request roles from User Group and User
Role Tab.](#30265)

#### Media
> A video or a GIF is preferred. when using Loom, don’t embed because it
looks like it’s a GIF. instead, just link to the video
>
>
#### Type of change
- Bug fix (non-breaking change which fixes an issue)

## Testing
>
#### How Has This Been Tested?
- [x] Manual
- [x] JUnit

#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [x] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed

---------

Co-authored-by: Nilesh Sarupriya <20905988+nsarupr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohanarpit mohanarpit mohanarpit left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] User sign up with POST request to the API
8 participants
@monarch0111 @mohanarpit @hetunandu @trishaanand @akash-codemonk @sharat87 @Nikhil-Nandagopal @riodeuno