Skip to content

fix: Add signup disabled flag check on signup route#15844

Merged
PeerRich merged 8 commits intocalcom:mainfrom
Souptik2001:fix-15758-check-signup-disable-flag-on-singup-api
Jul 30, 2024
Merged

fix: Add signup disabled flag check on signup route#15844
PeerRich merged 8 commits intocalcom:mainfrom
Souptik2001:fix-15758-check-signup-disable-flag-on-singup-api

Conversation

@Souptik2001
Copy link
Copy Markdown
Contributor

@Souptik2001 Souptik2001 commented Jul 20, 2024

What does this PR do?

(Just to note that in the below video I am refreshing the server again and again is just to refresh the cache, so that I don't have to wait for 5 mins.)

https://www.loom.com/share/5246b4b7e13b4b759086f74d57fa19d5?sid=9eadf523-4c58-440b-bdc6-31a96cee0152

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Create a POST request to signup API as demonstrated in the video. This should be successful.
  • Then disable the signup from the admin settings.
  • Wait for 5 mins. (cache expiration time)
  • Then again make the above request. This should now say that signup is disabled.

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>
@vercel
Copy link
Copy Markdown

vercel bot commented Jul 20, 2024

@Souptik2001 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot requested a review from a team July 20, 2024 20:56
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Jul 20, 2024
@github-actions github-actions bot added Low priority Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Jul 20, 2024
@dosubot dosubot bot added authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in core area: core, team members only labels Jul 20, 2024
@graphite-app
Copy link
Copy Markdown

graphite-app bot commented Jul 20, 2024

Graphite Automations

"Add community label" took an action on this PR • (07/20/24)

1 label was added to this PR based on Keith Williams's automation.

"Add consumer team as reviewer" took an action on this PR • (07/20/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (07/23/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@anikdhabal anikdhabal changed the title fix: [CAL-4038] Add signup disabled flag check on signup API fix: Add signup disabled flag check on signup API Jul 21, 2024
@anikdhabal anikdhabal added the high-risk Requires approval by Foundation team label Jul 23, 2024
@graphite-app graphite-app bot requested a review from a team July 23, 2024 06:25
@anikdhabal anikdhabal removed the core area: core, team members only label Jul 23, 2024
@keithwillcode keithwillcode changed the title fix: Add signup disabled flag check on signup API fix: Add signup disabled flag check on signup route Jul 24, 2024
Copy link
Copy Markdown
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Hey @Souptik2001, as we check the admin feature flag, shouldn't we also update the other routes that have related flags present in the admin feature? For example: email verification, team, etc.

@Souptik2001
Copy link
Copy Markdown
Contributor Author

@anikdhabal Yes you are correct!

Seems like the feature falgs are not respected at many places.

  • For email-verification, it is properly implemented. ✅
  • For emails, it is properly implemented. ✅
  • For organizations it is partially implemented. ⚠️
  • For teams, seems like it is not at all implemented? 🤔 As per I saw the teams feature was used nowhere (I may have missed also 🙏 )

For organizations -
I was not able to fully understand the purpose of this feature flag. I thought that it should disable the organizations feature completely. Is it meant for that only?
Because at some places (front-ends) (like creating a new organization), it redirects the page to 404 if this flag is disabled.
But majority front-ends are not handled, even the Organizations are visible in the left sidebar.
And the APIs are completely unprotected just like the signup one.
I have pushed a commit, which basically implements a middleware to check for the feature flag. Can you please check that.
I have implemented it in such a way that we can now protect any APIs (example - teams) using a feature flag.

If you can confirm the function of the organization feature flag, then I can go ahead and modify the front-end also to respect the flag.

And also if you can confirm that teams will need to be also respected.

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Jul 25, 2024

Hey @Souptik2001, as we check the admin feature flag, shouldn't we also update the other routes that have related flags present in the admin feature? For example: email verification, team, etc.

This is out of scope for this PR. We should open up different issues for each one.

@zomars zomars enabled auto-merge (squash) July 25, 2024 20:34
@zomars zomars disabled auto-merge July 25, 2024 20:35
@zomars
Copy link
Copy Markdown
Contributor

zomars commented Jul 25, 2024

I think this is good enough for now. As extra points we could add some tests to verify the signup page/endpoint are disabled tho.

@Souptik2001
Copy link
Copy Markdown
Contributor Author

As extra points we could add some tests to verify the signup page/endpoint are disabled tho.

Sure @zomars will do that in sometime.

@Souptik2001
Copy link
Copy Markdown
Contributor Author

hey @zomars sorry, was working on another issue (#15828), therefore couldn't get this one done.

Will do this one sometime tomorrow. 👍

@keithwillcode
Copy link
Copy Markdown
Contributor

Looks like there are consistently failing E2E tests we need to fix

@PeerRich PeerRich enabled auto-merge (squash) July 30, 2024 11:09
@PeerRich PeerRich merged commit 58cabe9 into calcom:main Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in 🐛 bug Something isn't working community Created by Linear-GitHub Sync high-risk Requires approval by Foundation team Low priority Created by Linear-GitHub Sync ready-for-e2e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants