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

Add functionality to enable/disable captcha #2796 #2810

Merged
merged 2 commits into from Aug 12, 2019

Conversation

fragm3
Copy link
Contributor

@fragm3 fragm3 commented Aug 10, 2019

Fixes #2796

Changes:

  • Add functionality to enable/disable captcha
  • Fix API key destructuring

Demo Link: https://pr-2810-fossasia-susi-web-chat.surge.sh

Screenshots of the change:

Screenshot 2019-08-10 at 9 58 13 PM

Copy link
Member

@akshatnitd akshatnitd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep a separate value for client side captcha. It would give us more control.

Let there be 2 -
isCaptchaEnabled - For server
isClientCaptchaEnabled - For client

@fragm3
Copy link
Contributor Author

fragm3 commented Aug 12, 2019

@akshatnitd for the captcha to verify on the server-side, it should always work on the client side, how else will we verify user captcha response.

Copy link
Member

@akshatnitd akshatnitd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier, we didn't have server side captcha validation. But, it can work, right.
What if, we don't want server side captcha revalidation, only client side. We can't control if we have only 1 flag.

@fragm3
Copy link
Contributor Author

fragm3 commented Aug 12, 2019

@akshatnitd that would be quite redundant 😿 , I'll do it if you say so

Copy link
Member

@akshatnitd akshatnitd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fragm3 I ll give you a use-case. Let me know, if it makes sense.

When the server side captcha was implemented, the Android login/signup started to fail. Also, there might be instances, when other clients might not have implemented captcha, and failed requests may happen.

These combinations are required for us -
Server - true; Client - true //best-case
Server - false, Client true
Server - false, Client - false //worst-case.

Does this make sense?

@fragm3
Copy link
Contributor Author

fragm3 commented Aug 12, 2019

@akshatnitd cross clients implementation makes sense, will do it 😸

@fragm3
Copy link
Contributor Author

fragm3 commented Aug 12, 2019

@akshatnitd made the changes.

@@ -20,6 +20,7 @@ class ResetPassword extends Component {
location: PropTypes.object,
actions: PropTypes.object,
history: PropTypes.object,
isCaptchaEnabled: PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isClientCaptchaEnabled

@fragm3
Copy link
Contributor Author

fragm3 commented Aug 12, 2019

@akshatnitd done

@akshatnitd akshatnitd merged commit 3cbae62 into fossasia:master Aug 12, 2019
@mariobehling
Copy link
Member

Sorry, but I need to revert this. You have not actually solved the issue. You solved something else that is not related to the issue. Not sure, what is your idea here. Please read the issue again.

pulkit1joshi pushed a commit to pulkit1joshi/susi.ai that referenced this pull request Jan 20, 2020
pulkit1joshi pushed a commit to pulkit1joshi/susi.ai that referenced this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin Message Tab: Add "Captcha" column with checkbox
3 participants