-
-
Notifications
You must be signed in to change notification settings - Fork 227
[feature] Made RegisteredUser model support multi-tenancy #692 #698
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
7470e3d
c12902d
13d559c
7eb6594
7157409
3b4c451
e478092
dd7228a
13295f0
6e72be3
718ab8d
69bff69
990c037
e6b2c4c
4dde443
931129e
0f904c3
fca0267
3887c2f
adf2fda
4d77cf6
d264d7a
24ea052
29857b0
4558a74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,7 @@ jobs: | |
| pip install -U pip wheel setuptools | ||
| pip install -U -r requirements-test.txt | ||
| pip install -e .[saml,openvpn_status] | ||
| pip install --upgrade --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Remove this before merging! |
||
| pip install ${{ matrix.django-version }} | ||
|
|
||
| - name: Start InfluxDB and Redis container | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -538,7 +538,11 @@ class RegisteredUserInline(StackedInline): | |
| model = RegisteredUser | ||
| form = AlwaysHasChangedForm | ||
| extra = 0 | ||
| readonly_fields = ("modified",) | ||
| readonly_fields = ( | ||
| "organization", | ||
| "modified", | ||
| ) | ||
| fields = ("organization", "method", "is_verified", "modified") | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| def has_delete_permission(self, request, obj=None): | ||
| return False | ||
|
|
@@ -549,12 +553,17 @@ def has_delete_permission(self, request, obj=None): | |
| RadiusUserGroupInline, | ||
| PhoneTokenInline, | ||
| ] | ||
| UserAdmin.list_filter += (RegisteredUserFilter, "registered_user__method") | ||
| UserAdmin.list_filter += (RegisteredUserFilter, "registered_users__method") | ||
|
|
||
|
|
||
| def get_is_verified(self, obj): | ||
| try: | ||
| value = "yes" if obj.registered_user.is_verified else "no" | ||
| if not obj.registered_users.exists(): | ||
| value = "unknown" | ||
| elif obj.registered_users.filter(is_verified=True).exists(): | ||
| value = "yes" | ||
| else: | ||
| value = "no" | ||
|
Comment on lines
575
to
+582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid N+1 queries in This now does up to two 🤖 Prompt for AI Agents |
||
| except Exception: | ||
| value = "unknown" | ||
| icon_url = static(f"admin/img/icon-{value}.svg") | ||
|
|
@@ -564,7 +573,6 @@ def get_is_verified(self, obj): | |
| UserAdmin.get_is_verified = get_is_verified | ||
| UserAdmin.get_is_verified.short_description = _("Verified") | ||
| UserAdmin.list_display.insert(3, "get_is_verified") | ||
| UserAdmin.list_select_related = ("registered_user",) | ||
|
|
||
|
|
||
| class OrganizationRadiusSettingsInline(admin.StackedInline): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,7 @@ | |
| Organization = swapper.load_model("openwisp_users", "Organization") | ||
| OrganizationUser = swapper.load_model("openwisp_users", "OrganizationUser") | ||
| PhoneToken = load_model("PhoneToken") | ||
| RegisteredUser = load_model("RegisteredUser") | ||
| RadiusAccounting = load_model("RadiusAccounting") | ||
| RadiusToken = load_model("RadiusToken") | ||
| RadiusBatch = load_model("RadiusBatch") | ||
|
|
@@ -315,13 +316,13 @@ def post(self, request, *args, **kwargs): | |
| self.update_user_details(user) | ||
| context = {"view": self, "request": request} | ||
| serializer = self.serializer_class(instance=token, context=context) | ||
| response = RadiusUserSerializer(user).data | ||
| response = RadiusUserSerializer(user, context=context).data | ||
| response.update(serializer.data) | ||
| status_code = 200 if user.is_active else 401 | ||
| # If identity verification is required, check if user is verified | ||
| if self._needs_identity_verification( | ||
| {"slug": kwargs["slug"]} | ||
| ) and not self.is_identity_verified_strong(user): | ||
| ) and not self.is_identity_verified_strong(user, self.organization): | ||
| status_code = 401 | ||
| return Response(response, status=status_code) | ||
|
|
||
|
|
@@ -335,24 +336,24 @@ def validate_membership(self, user): | |
| if get_organization_radius_settings( | ||
| self.organization, "registration_enabled" | ||
| ): | ||
| if self._needs_identity_verification( | ||
| org=self.organization | ||
| ) and not self.is_identity_verified_strong(user): | ||
| raise PermissionDenied | ||
| try: | ||
| org_user = OrganizationUser( | ||
| user=user, organization=self.organization | ||
| ) | ||
| org_user.full_clean() | ||
| org_user.save() | ||
| RegisteredUser.objects.get_or_create( | ||
| user=user, | ||
| organization=self.organization, | ||
| defaults={"method": ""}, | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| except ValidationError as error: | ||
| raise serializers.ValidationError( | ||
| {"non_field_errors": error.message_dict.pop("__all__")} | ||
| ) | ||
| else: | ||
| message = _( | ||
| "{organization} does not allow self registration " | ||
| "of new accounts." | ||
| "{organization} does not allow self registration of new accounts." | ||
| ).format(organization=self.organization.name) | ||
| raise PermissionDenied(message) | ||
|
|
||
|
|
@@ -383,9 +384,15 @@ def post(self, request, *args, **kwargs): | |
| response = {"response_code": "BLANK_OR_INVALID_TOKEN"} | ||
| if request_token: | ||
| try: | ||
| token = UserToken.objects.select_related( | ||
| "user", "user__registered_user" | ||
| ).get(key=request_token) | ||
| token = ( | ||
| UserToken.objects.select_related( | ||
| "user", | ||
| ) | ||
| .prefetch_related( | ||
| "user__registered_users", | ||
| ) | ||
| .get(key=request_token) | ||
| ) | ||
| except UserToken.DoesNotExist: | ||
| pass | ||
| else: | ||
|
|
@@ -395,7 +402,7 @@ def post(self, request, *args, **kwargs): | |
| ) | ||
| # user may be in the process of changing the phone number | ||
| # in that case show the new phone number (which is not verified yet) | ||
| if not self.is_identity_verified_strong(user): | ||
| if not self.is_identity_verified_strong(user, self.organization): | ||
| phone_token = ( | ||
| PhoneToken.objects.filter(user=user) | ||
| .order_by("-created") | ||
|
|
@@ -404,8 +411,8 @@ def post(self, request, *args, **kwargs): | |
| user.phone_number = ( | ||
| phone_token.phone_number if phone_token else user.phone_number | ||
| ) | ||
| response = RadiusUserSerializer(user).data | ||
| context = {"view": self, "request": request} | ||
| response = RadiusUserSerializer(user, context=context).data | ||
| token_data = rest_auth_settings.api_settings.TOKEN_SERIALIZER( | ||
| token, context=context | ||
| ).data | ||
|
|
@@ -614,11 +621,13 @@ class CreatePhoneTokenView( | |
| ) | ||
|
|
||
| @swagger_auto_schema( | ||
| operation_description=(""" | ||
| operation_description=( | ||
| """ | ||
| **Requires the user auth token (Bearer Token).** | ||
| Used for SMS verification, sends a code via SMS to the | ||
| phone number of the user. | ||
| """), | ||
| """ | ||
| ), | ||
| request_body=no_body, | ||
| responses={201: ""}, | ||
| ) | ||
|
|
@@ -638,7 +647,7 @@ def create(self, *args, **kwargs): | |
| try: | ||
| phone_token.full_clean() | ||
| if kwargs.get("enforce_unverified", True): | ||
| phone_token._validate_already_verified() | ||
| phone_token._validate_already_verified(organization=self.organization) | ||
| except ValidationError as e: | ||
| error_dict = self._get_error_dict(e) | ||
| raise serializers.ValidationError(error_dict) | ||
|
|
@@ -692,12 +701,14 @@ class GetPhoneTokenStatusView(DispatchOrgMixin, GenericAPIView): | |
| serializer_class = serializers.Serializer | ||
|
|
||
| @swagger_auto_schema( | ||
| operation_description=(""" | ||
| operation_description=( | ||
| """ | ||
| **Requires the user auth token (Bearer Token).** | ||
| Used for SMS verification, allows checking whether an active | ||
| SMS token was already requested for the mobile phone number | ||
| of the logged in account. | ||
| """), | ||
| """ | ||
| ), | ||
| responses={200: '`{"active":"true/false"}`'}, | ||
| ) | ||
| def get(self, request, *args, **kwargs): | ||
|
|
@@ -747,23 +758,33 @@ def post(self, request, *args, **kwargs): | |
| _("No verification code found in the system for this user.") | ||
| ) | ||
| try: | ||
| is_valid = phone_token.is_valid(serializer.data["code"]) | ||
| is_valid = phone_token.is_valid( | ||
| serializer.data["code"], organization=self.organization | ||
| ) | ||
| except PhoneTokenException as e: | ||
| return self._error_response(str(e)) | ||
| if not is_valid: | ||
| return self._error_response(_("Invalid code.")) | ||
| else: | ||
| user.registered_user.is_verified = True | ||
| user.registered_user.method = "mobile_phone" | ||
| user.is_active = True | ||
| reg_user, __ = RegisteredUser.get_or_create_for_user_and_org( | ||
| user=user, | ||
| organization=self.organization, | ||
| defaults={ | ||
| "is_verified": True, | ||
| "method": "mobile_phone", | ||
| "is_active": True, | ||
| }, | ||
| ) | ||
| reg_user.is_verified = True | ||
| reg_user.method = "mobile_phone" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really required? |
||
| # Update username if phone_number is used as username | ||
| if user.username == user.phone_number: | ||
| user.username = phone_token.phone_number | ||
| # now that the phone number is verified | ||
| # we can write it to the user field | ||
| user.phone_number = phone_token.phone_number | ||
| user.save() | ||
| user.registered_user.save() | ||
| reg_user.save() | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| # delete any radius token cache key if present | ||
| cache.delete(f"rt-{phone_token.phone_number}") | ||
| return Response(None, status=200) | ||
|
|
@@ -781,11 +802,13 @@ class ChangePhoneNumberView(ThrottledAPIMixin, CreatePhoneTokenView): | |
| serializer_class = ChangePhoneNumberSerializer | ||
|
|
||
| @swagger_auto_schema( | ||
| operation_description=(""" | ||
| operation_description=( | ||
| """ | ||
| **Requires the user auth token (Bearer Token).** | ||
| Allows users to change their phone number, will flag the | ||
| user as inactive and send them a verification code via SMS. | ||
| """), | ||
| """ | ||
| ), | ||
| responses={200: ""}, | ||
| ) | ||
| def post(self, request, *args, **kwargs): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
--no-depsflag bypasses dependency resolution and risks broken environments.Installing
openwisp-userswith--no-depsprevents pip from checking or installing its dependencies. If theissues/497-export-usersbranch requires different versions of shared dependencies (Django, other OpenWISP packages, etc.), those won't be resolved, potentially causing runtime failures that only surface during test execution rather than at install time.🔧 Safer installation approach
If
--no-depsis intentionally used to avoid dependency conflicts, document why and verify that all required dependencies are already correctly installed by the previous steps.📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai this is temporary, ignore!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pandafy, understood! I'll disregard this comment. The temporarypip install --no-depsline is a workaround pending the merge of the upstreamopenwisp-usersPR#498.✏️ Learnings added
🧠 Learnings used