fix: prevent signup blocking invitation signups#5507
Conversation
Zaimwa9
requested review from
emyller and
tiagoapolo
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
Bot
added
front-end
Docker builds report
|
Zaimwa9
changed the title
for more information, see https://pre-commit.ci
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/5507 ⚙️ Updating now by workflow run 15324374777. What is Uffizzi? Learn more! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5507 +/- ##
=======================================
Coverage 97.65% 97.65%
=======================================
Files 1237 1238 +1
Lines 43580 43634 +54
=======================================
+ Hits 42558 42612 +54
Misses 1022 1022 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try: | ||
| invite_link = InviteLink.objects.get(hash=invite_hash) | ||
| if not invite_link.is_expired: | ||
| return True | ||
| except InviteLink.DoesNotExist: | ||
| pass |
There was a problem hiding this comment.
| try: | |
| invite_link = InviteLink.objects.get(hash=invite_hash) | |
| if not invite_link.is_expired: | |
| return True | |
| except InviteLink.DoesNotExist: | |
| pass | |
| try: | |
| invite_link = InviteLink.objects.get(hash=invite_hash) | |
| except InviteLink.DoesNotExist: | |
| return False | |
| return not invite_link.is_expired |
This keeps only the code that is expected to fail within the try clause, and also returns False rather than None — if that's ideal.
There was a problem hiding this comment.
Agreed to extract the is_expired check from the try/catch block - thanks.
Allow me to disagree on the False/None part. It should not be returning None as it would:
- return true (allow-all, valid email, valid link)
- surface a server/unknown error
- if doesNotExist or is_expired => passes through the PermissionDenied.
Which to me is the correct error to return.
There was a problem hiding this comment.
Also for context, DRF will short-circuit to a 401 if authentication fails (via get_authenticators()) without explicitely raising an error
There was a problem hiding this comment.
Thanks for the context.
Sorry I addressed the raise statement in another comment.
As per DRF documentation, it will automatically raise PermissionDenied when has_permission returns False. Having it manually raise feels like an anti-pattern. If you need to customize how DRF will present that exception to the HTTP response, you may set the attributes code and message to the permission class.
There's also this SO thread with examples.
Let me know if this helps.
|
|
||
|
|
||
| class IsSignupAllowed(AllowAny): | ||
| message = "Signing-up without a valid invitation is disabled. Please contact your administrator." |
There was a problem hiding this comment.
This message sounds too specific for a permission with a generic name. 🤔
Perhaps we should use it in the code branch within has_permission?
There was a problem hiding this comment.
Yes I thought the same but that's actually the only purpose of this permission afaik:
PREVENT_SIGNUP = false=> Always allowedPREVENT_SIGNUP = true=> Needs invitation or error
There was a problem hiding this comment.
I see. In that case, just a minor tweak:
| message = "Signing-up without a valid invitation is disabled. Please contact your administrator." | |
| message = "Signing up without an invitation is disabled. Please contact your administrator." |
| except InviteLink.DoesNotExist: | ||
| pass | ||
|
|
||
| raise PermissionDenied(self.message) |
There was a problem hiding this comment.
Following returning False above, DRF should already raise using the message attribute from the class on its own. 😉
|
Closed in favor of #5508 |
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!closes #5506
Changes
IsSignupAllowedpermissionsHow did you test this code?
To test:
"PREVENT_SIGNUP": "1",https://www.loom.com/share/02c75a8dc28c4378b9856867c2ca7c85