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

[RHCLOUD-37601] Store lower case username for principal #1472

Merged
merged 1 commit into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions rbac/internal/specs/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,84 @@
}
}
}
},
"/api/utils/username_lower/": {
"get": {
"tags": [
"List username"
],
"summary": "List uppercase username",
"description": "List uppercase username.",
"operationId": "ListUsername",
"responses": {
"200": {
"description": "List of uppercase username."
},
"405": {
"description": "Invalid method.",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/Error"
}
}
}
},
"500": {
"description": "Unexpected Error",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/Error"
}
}
}
}
}
},
"post": {
"tags": [
"Update username"
],
"summary": "Update uppercase username to lowercase",
"description": "Update uppercase username to lowercase.",
"operationId": "LowerUsername",
"responses": {
"200": {
"description": "All uppercase username updated to lowercase."
},
"400":{
"description": "Invalid request.",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/Error"
}
}
}
},
"405": {
"description": "Invalid method.",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/Error"
}
}
}
},
"500": {
"description": "Unexpected Error",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/Error"
}
}
}
}
}
}
}
},
"servers": [
Expand Down
1 change: 1 addition & 0 deletions rbac/internal/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
path("api/utils/get_org_admin/<int:org_or_account>/", views.get_org_admin),
path("api/utils/role/", views.role_removal),
path("api/utils/permission/", views.permission_removal),
path("api/utils/username_lower/", views.username_lower),
path("api/utils/data_migration/", views.data_migration),
path("api/utils/bindings/<role_uuid>/", views.list_or_delete_bindings_for_role),
path("api/utils/bootstrap_tenant/", views.bootstrap_tenant),
Expand Down
33 changes: 32 additions & 1 deletion rbac/internal/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@
from django.conf import settings
from django.db import connection, transaction
from django.db.migrations.recorder import MigrationRecorder
from django.db.models import Func
from django.http import HttpRequest, HttpResponse
from django.shortcuts import get_object_or_404
from django.utils.html import escape
from internal.utils import delete_bindings
from management.cache import TenantCache
from management.models import Group, Permission, ResourceDefinition, Role
from management.models import Group, Permission, Principal, ResourceDefinition, Role
from management.principal.proxy import (
API_TOKEN_HEADER,
CLIENT_ID_HEADER,
Expand Down Expand Up @@ -865,3 +866,33 @@ def correct_resource_definitions(request):
return HttpResponse(f"Updated {count} bad resource definitions", status=200)

return HttpResponse('Invalid method, only "GET" or "PATCH" are allowed.', status=405)


class Upper(Func):
"""Upper class function."""

function = "UPPER"


def username_lower(request):
"""Update the username for the principal to be lowercase."""
if request.method not in ["POST", "GET"]:
return HttpResponse("Invalid request method, only POST/GET are allowed.", status=405)
if request.method == "POST" and not destructive_ok("api"):
return HttpResponse("Destructive operations disallowed.", status=400)

pre_names = []
updated_names = []
with transaction.atomic():
principals = Principal.objects.filter(type="user").filter(username=Upper("username"))
for principal in principals:
pre_names.append(principal.username)
principal.username = principal.username.lower()
updated_names.append(principal.username)
if request.method == "GET":
return HttpResponse(
f"Usernames to be updated: {pre_names} to {updated_names}",
status=200,
)
Principal.objects.bulk_update(principals, ["username"])
return HttpResponse(f"Updated {len(principals)} usernames", status=200)
5 changes: 5 additions & 0 deletions rbac/management/principal/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ def principal_resource_id(self) -> Optional[str]:
return None
return Principal.user_id_to_principal_resource_id(self.user_id)

def save(self, *args, **kwargs):
"""Override save to only store lower case username."""
self.username = self.username.lower()
super(Principal, self).save(*args, **kwargs)

class Meta:
ordering = ["username"]
constraints = [
Expand Down
38 changes: 38 additions & 0 deletions tests/internal/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1316,6 +1316,44 @@ def test_fetch_role(self):
self.assertFalse(role["admin_default"])
self.assertEqual(response.status_code, 200)

@override_settings(INTERNAL_DESTRUCTIVE_API_OK_UNTIL=valid_destructive_time())
def test_update_username_to_lowercase(self):
"""Test that the uppercase username would be updated to lowercase."""
# Only POST is allowed
response = self.client.delete(
f"/_private/api/utils/username_lower/",
**self.request.META,
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)

Principal.objects.bulk_create(
[
Principal(username="12345", tenant=self.tenant),
Principal(username="ABCDE", tenant=self.tenant),
Principal(username="user", tenant=self.tenant),
]
)

response = self.client.get(
f"/_private/api/utils/username_lower/",
**self.request.META,
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(
response.content.decode(), "Usernames to be updated: ['12345', 'ABCDE'] to ['12345', 'abcde']"
)

response = self.client.post(
f"/_private/api/utils/username_lower/",
**self.request.META,
content_type="application/json",
)
self.assertEqual(response.status_code, 200)
usernames = Principal.objects.values_list("username", flat=True)
self.assertEqual({"12345", "abcde", "user"}, set(usernames))


class InternalViewsetResourceDefinitionTests(IdentityRequest):
def setUp(self):
Expand Down
4 changes: 2 additions & 2 deletions tests/management/principal/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ def test_principal_creation(self):
"""Test that we can create principal correctly."""
# Default value for cross_account is False.
principalA = Principal.objects.create(username="principalA", tenant=self.tenant)
self.assertEqual(principalA.username, "principalA")
self.assertEqual(principalA.username, "principala")
self.assertEqual(principalA.cross_account, False)

# Explicitly set cross_account.
principalB = Principal.objects.create(username="principalB", cross_account=True, tenant=self.tenant)
self.assertEqual(principalB.username, "principalB")
self.assertEqual(principalB.username, "principalb")
self.assertEqual(principalB.cross_account, True)