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

Centralize demo mode DB clearing logic in ExperimentUserService #1845

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

zackcl
Copy link
Collaborator

@zackcl zackcl commented Aug 21, 2024

This PR centralizes the logic for clearing the database (/clearDB) in demo mode within the ExperimentUserService.

Key changes:

  • Moved the demo mode check from ExperimentClientController to ExperimentUserService
  • Updated clearDB method in ExperimentUserService to include demo mode check
  • Simplified /clearDB endpoints in all versions of ExperimentClientController

Benefits:

  • Improves code maintainability by centralizing demo mode logic
  • Reduces risk of inconsistencies across different controller versions

Testing:

  • Verified that clearDB functionality works as expected in demo mode
  • Confirmed that clearDB is properly restricted when not in demo mode

@zackcl zackcl added enhancement New feature or request backend labels Aug 21, 2024
@zackcl zackcl self-assigned this Aug 21, 2024
@zackcl
Copy link
Collaborator Author

zackcl commented Aug 21, 2024

@danoswaltCL I've added the @Authorized() decorator to the /clearDB endpoint for extra security as you suggested. I've fully tested the endpoint in different versions (v1, v4, v5) locally, and it works as expected. Please feel free to approve this PR.

@danoswaltCL
Copy link
Collaborator

wait... this endpoint shouldn't be on the client controller, that makes no sense, especially now if using an authenticated endpoint middleware that uses a different auth than the client middleware uses. clients don't call this, it's an admin endpoint that should require a google login user credential.

Maybe can we copy paste this endpoint over to some other spot like the SettingsService and delete if from all client controllers? Kinda feels like that would be a better fit (if we really have to have this endpoint).

@VivekFitkariwala @bcb37 @ppratikcr7 any thoughts? this came about from me asking Zack if we really needed this endpoint, it seems like we're flirting with danger

@bcb37
Copy link
Collaborator

bcb37 commented Aug 21, 2024

wait... this endpoint shouldn't be on the client controller, that makes no sense, especially now if using an authenticated endpoint middleware that uses a different auth than the client middleware uses. clients don't call this, it's an admin endpoint that should require a google login user credential.

Maybe can we copy paste this endpoint over to some other spot like the SettingsService and delete if from all client controllers? Kinda feels like that would be a better fit (if we really have to have this endpoint).

@VivekFitkariwala @bcb37 @ppratikcr7 any thoughts? this came about from me asking Zack if we really needed this endpoint, it seems like we're flirting with danger

It didn't feel like a huge change. It's technically a bit safer because the Authorization decorator is requiring the same authorization as the non-client endpoints. It's an odd endpoint so it's not obvious where it belongs. Maybe it should have its own controller? But if we're going to revisit the need for this eventually anyway, I was ok with leaving it in the client controller with Authorization required. It's necessary for the demo app to function without a lot of re-engineering,

@VivekFitkariwala
Copy link
Collaborator

As @bcb37 said this is an odd endpoint and it is present because we don't have a multitenant system. So I don't have a strong opinion. We must be sure that the DEMO flag is false in production :)

@zackcl zackcl merged commit 056283f into dev Aug 22, 2024
8 checks passed
@zackcl zackcl deleted the feature/cleardb-demo-check branch August 22, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants