Skip to content

[Reputation Oracle] hCaptcha token validation moved to the HCaptchaGuard #2999

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
Merged
Dzeranov merged 5 commits into from
Jan 17, 2025
Merged

[Reputation Oracle] hCaptcha token validation moved to the HCaptchaGuard #2999

Dzeranov merged 5 commits into from
Jan 17, 2025

Conversation

@Dzeranov
Copy link
Contributor

@Dzeranov Dzeranov commented Jan 13, 2025 •
edited
Loading

Issue tracking

To close #2995

Context behind the change

hCaptcha token validation moved to the HCaptchaGuard to remove code duplications and simplify services logic.

How has this been tested?

  • Reputation Oracle locally deployed and manually tested all the affected endpoints

Release plan

  • Add a value for HUMAN_APP_EMAIL in the environment.

Potential risks; What to monitor; Rollback plan

N/A

Please, do not merge until this PR is merged.

@Dzeranov Dzeranov self-assigned this Jan 13, 2025
@vercel
Copy link

vercel bot commented Jan 13, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 10:43am
human-dashboard-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 10:43am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
faucet-frontend ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 10:43am
faucet-server ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 10:43am

@Dzeranov Dzeranov changed the title hCaptcha token validation moved to the HCaptchaGuard [Reputation Oracle] hCaptcha token validation moved to the HCaptchaGuard Jan 13, 2025
@Dzeranov
fix: @UseGuards used separately for each endpoint in User control…
4d88e27 
…ler to maintain the correct order of guards (`HCaptchaGuard` before `JwtAuthGuard`)
@Dzeranov Dzeranov requested a review from dnechay January 15, 2025 12:35
@Dzeranov Dzeranov marked this pull request as ready for review January 15, 2025 12:35
@Dzeranov Dzeranov requested a review from portuu3 January 15, 2025 12:35
dnechay
dnechay reviewed Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@dnechay dnechay left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions

After moving captcha verification logic to its own guard we can clearly see that e.g. SignInDto is not proper type of AuthService.signIn because it contains excessive information like captcha token. Ideally, DTO should be used when transferring data (e.g. for request and response bodies and proper conversion should be done in place) and services should have their own types, but it's a separate topic, not related to this PR

@Dzeranov
Copy link
Contributor Author

Dzeranov commented Jan 16, 2025

LGTM, left some suggestions

After moving captcha verification logic to its own guard we can clearly see that e.g. SignInDto is not proper type of AuthService.signIn because it contains excessive information like captcha token. Ideally, DTO should be used when transferring data (e.g. for request and response bodies and proper conversion should be done in place) and services should have their own types, but it's a separate topic, not related to this PR

Agree with that but let it be a part of another refactoring phase.

@Dzeranov Dzeranov requested a review from dnechay January 16, 2025 10:41
@Dzeranov Dzeranov added the do-not-merge PR shouldn't be merged until this label is removed label Jan 16, 2025
@Dzeranov Dzeranov merged commit 2ad0673 into develop Jan 17, 2025
11 checks passed
@Dzeranov Dzeranov mentioned this pull request Jan 17, 2025
Merged
35 tasks
@Dzeranov Dzeranov removed the do-not-merge PR shouldn't be merged until this label is removed label Jan 17, 2025
@Dzeranov Dzeranov deleted the dzeranov/2995 branch January 17, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnechay dnechay dnechay approved these changes

@portuu3 portuu3 portuu3 approved these changes

Assignees

@Dzeranov Dzeranov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Reputation Oracle] Implement hCaptcha guard

4 participants

@Dzeranov @dnechay @portuu3