feat: add profile edit
Profile page at /profile with first name, last name, display name, telephone and mobile read from AD and shown with the username and e-mail. Only changed fields are written through the directory adapter in one transaction with the audit entry (field names only); numbers are accepted in any common notation and stored as +country code and digits; names are required. Plain-language errors per field, German text and tests; review findings logged in the plan. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FocPJ75XPma7TcuYEMy8zm
This commit is contained in:
Ebbe Baß
co-authored by
Claude Sonnet 5.5
Claude Sonnet 5.5
parent
ae4b30ec22
commit
3fd8f27127
@@ -1,4 +1,8 @@
|
||||
import re
|
||||
import unicodedata
|
||||
|
||||
from django import forms
|
||||
from django.utils.translation import gettext
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
||||
MAX_PASSWORD_LENGTH = 256 # Active Directory's limit
|
||||
@@ -33,3 +37,129 @@ class PasswordChangeForm(forms.Form):
|
||||
elif new and current and new == current:
|
||||
self.add_error("new_password", _("The new password must be different."))
|
||||
return data
|
||||
|
||||
|
||||
# -- profile details --------------------------------------------------------------------------
|
||||
|
||||
# Form field name -> (label, maximum length (AD's limit), required)
|
||||
DETAIL_FIELDS = {
|
||||
"first_name": (_("First name"), 64, True),
|
||||
"last_name": (_("Last name"), 64, True),
|
||||
"display_name": (_("Display name"), 256, True),
|
||||
"telephone": (_("Telephone"), 64, False),
|
||||
"mobile": (_("Mobile"), 64, False),
|
||||
}
|
||||
PHONE_FIELDS = ("telephone", "mobile")
|
||||
PHONE_MIN_DIGITS = 7
|
||||
PHONE_MAX_DIGITS = 15
|
||||
DEFAULT_COUNTRY_CODE = "+49" # a leading 0 is read as Germany
|
||||
|
||||
_PHONE_SEPARATORS = re.compile(r"[ \-/.()]")
|
||||
_PHONE_SHAPE = re.compile(r"\+?[0-9]+")
|
||||
|
||||
|
||||
class InvalidNumber(ValueError):
|
||||
"""The text cannot be read as a telephone number."""
|
||||
|
||||
|
||||
def normalize_phone(text: str) -> str:
|
||||
"""The one stored form of a telephone number: `+` and digits only.
|
||||
|
||||
Spaces, `-`, `/`, `.` and brackets are removed and a `(0)` after a country code is dropped.
|
||||
`00` becomes `+`, a single leading `0` becomes `+49`. Anything else raises InvalidNumber.
|
||||
The result is its own normal form, so normalizing twice changes nothing.
|
||||
"""
|
||||
text = text.strip()
|
||||
if text.startswith(("+", "00")):
|
||||
text = text.replace("(0)", "")
|
||||
text = _PHONE_SEPARATORS.sub("", text)
|
||||
if not _PHONE_SHAPE.fullmatch(text):
|
||||
raise InvalidNumber(text)
|
||||
if text.startswith("00"):
|
||||
text = "+" + text[2:]
|
||||
elif text.startswith("0"):
|
||||
text = DEFAULT_COUNTRY_CODE + text[1:]
|
||||
elif not text.startswith("+"):
|
||||
raise InvalidNumber(text)
|
||||
if not PHONE_MIN_DIGITS <= len(text) - 1 <= PHONE_MAX_DIGITS:
|
||||
raise InvalidNumber(text)
|
||||
return text
|
||||
|
||||
|
||||
def _has_control_character(text: str) -> bool:
|
||||
return any(unicodedata.category(ch) in ("Cc", "Zl", "Zp") for ch in text)
|
||||
|
||||
|
||||
def _check_one(name: str, value: str) -> str:
|
||||
"""The cleaned value of a changed field, or a ValueError carrying the message."""
|
||||
label, max_length, required = DETAIL_FIELDS[name]
|
||||
if not value:
|
||||
if required:
|
||||
raise ValueError(gettext("%(field)s is required.") % {"field": label})
|
||||
return ""
|
||||
if _has_control_character(value):
|
||||
raise ValueError(
|
||||
gettext("%(field)s must not contain line breaks or control characters.")
|
||||
% {"field": label}
|
||||
)
|
||||
if len(value) > max_length:
|
||||
raise ValueError(
|
||||
gettext("%(field)s must not be longer than %(max)s characters.")
|
||||
% {"field": label, "max": max_length}
|
||||
)
|
||||
if name in PHONE_FIELDS:
|
||||
try:
|
||||
return normalize_phone(value)
|
||||
except InvalidNumber:
|
||||
raise ValueError(
|
||||
gettext(
|
||||
"%(field)s must be a number that starts with + and the country code, or with 0."
|
||||
)
|
||||
% {"field": label}
|
||||
) from None
|
||||
return value
|
||||
|
||||
|
||||
def check_details(
|
||||
values: dict[str, str], current: dict[str, str]
|
||||
) -> tuple[dict[str, str], dict[str, str]]:
|
||||
"""Check submitted values against what the directory holds now.
|
||||
|
||||
Returns (cleaned, errors). A field whose trimmed text equals the directory value is neither
|
||||
checked nor changed, so one old value in the directory never blocks saving another field.
|
||||
A number written in another way but equal once normalized is unchanged too.
|
||||
"""
|
||||
cleaned: dict[str, str] = {}
|
||||
errors: dict[str, str] = {}
|
||||
for name in DETAIL_FIELDS:
|
||||
value = (values.get(name) or "").strip()
|
||||
existing = current.get(name) or ""
|
||||
if value == existing.strip():
|
||||
cleaned[name] = existing
|
||||
continue
|
||||
try:
|
||||
result = _check_one(name, value)
|
||||
except ValueError as exc:
|
||||
errors[name] = str(exc)
|
||||
continue
|
||||
cleaned[name] = existing if result == existing.strip() else result
|
||||
return cleaned, errors
|
||||
|
||||
|
||||
class _TextField(forms.CharField):
|
||||
"""Trims only: the checks need the directory value and live in `check_details`."""
|
||||
|
||||
def run_validators(self, value):
|
||||
pass
|
||||
|
||||
|
||||
class DetailsForm(forms.Form):
|
||||
first_name = _TextField(required=False, strip=True)
|
||||
last_name = _TextField(required=False, strip=True)
|
||||
display_name = _TextField(required=False, strip=True)
|
||||
telephone = _TextField(required=False, strip=True)
|
||||
mobile = _TextField(required=False, strip=True)
|
||||
|
||||
def apply_errors(self, errors: dict[str, str]) -> None:
|
||||
for name, message in errors.items():
|
||||
self.add_error(name, message)
|
||||
|
||||
+149
-3
@@ -1,4 +1,4 @@
|
||||
"""Password change for the signed-in user (FR-5, FR-6, FR-7; AD-3, AD-4, AD-8).
|
||||
"""Password change and profile details of the signed-in user (FR-5..FR-9; AD-3, AD-4, AD-8).
|
||||
|
||||
The order is fixed: throttle check, policy read, verification of the current password, local
|
||||
hints, then the directory change, the ended sessions, the audit entry and the queued mail in
|
||||
@@ -10,7 +10,7 @@ from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import math
|
||||
from dataclasses import dataclass
|
||||
from dataclasses import dataclass, replace
|
||||
from datetime import timedelta
|
||||
from zoneinfo import ZoneInfo
|
||||
|
||||
@@ -22,11 +22,20 @@ from django.utils.translation import ngettext
|
||||
|
||||
from portal.accounts import throttle
|
||||
from portal.accounts.sessions import SessionUser, end_sessions
|
||||
from portal.adapters.directory import DirectoryUnavailable, PasswordPolicy, PasswordRejected
|
||||
from portal.adapters.directory import (
|
||||
DirectoryUnavailable,
|
||||
DirectoryUser,
|
||||
Forbidden,
|
||||
PasswordPolicy,
|
||||
PasswordRejected,
|
||||
ValidationFailed,
|
||||
)
|
||||
from portal.adapters.registry import get_directory
|
||||
from portal.audit import services as audit
|
||||
from portal.mailer import services as mailer
|
||||
|
||||
from .forms import DETAIL_FIELDS, check_details
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
ENDPOINT = "password-change"
|
||||
@@ -309,3 +318,140 @@ def _queue_mail(account) -> bool:
|
||||
{"name": name or account.sam, "changed_at": now, "zone_name": settings.DISPLAY_TIME_ZONE},
|
||||
)
|
||||
return True
|
||||
|
||||
|
||||
# -- profile details (FR-9) -------------------------------------------------------------------
|
||||
|
||||
# Form field -> (directory attribute, DirectoryUser field). Only these five are ever written.
|
||||
DETAIL_ATTRIBUTES = {
|
||||
"first_name": ("givenName", "given_name"),
|
||||
"last_name": ("sn", "surname"),
|
||||
"display_name": ("displayName", "display_name"),
|
||||
"telephone": ("telephoneNumber", "telephone"),
|
||||
"mobile": ("mobile", "mobile"),
|
||||
}
|
||||
|
||||
|
||||
class DetailsInvalid(Exception):
|
||||
"""Values were refused before anything was written; `errors` maps a field to its message."""
|
||||
|
||||
def __init__(self, errors: dict[str, str], details: Details) -> None:
|
||||
self.errors = errors
|
||||
self.details = details # what the one directory read held, for the page
|
||||
super().__init__("The details are not valid")
|
||||
|
||||
|
||||
class DetailsNotAccepted(Exception):
|
||||
"""The directory refused a value."""
|
||||
|
||||
def __init__(self, details: Details) -> None:
|
||||
self.details = details
|
||||
super().__init__("The directory refused a value")
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class Details:
|
||||
username: str
|
||||
mail: str
|
||||
values: dict[str, str]
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class DetailsUpdated:
|
||||
changed: tuple[str, ...] # form field names, in page order
|
||||
account: DirectoryUser # the user as the directory now holds them
|
||||
|
||||
|
||||
def _values_of(account: DirectoryUser) -> dict[str, str]:
|
||||
return {name: getattr(account, attr) or "" for name, (_attr, attr) in DETAIL_ATTRIBUTES.items()}
|
||||
|
||||
|
||||
def _read(user: SessionUser) -> DirectoryUser:
|
||||
account = get_directory().find_user(user.sam)
|
||||
if account is None:
|
||||
raise DirectoryUnavailable("The account could not be read")
|
||||
return account
|
||||
|
||||
|
||||
def load_details(user: SessionUser) -> Details:
|
||||
"""What the directory holds about the signed-in user. Raises DirectoryUnavailable."""
|
||||
account = _read(user)
|
||||
return Details(username=account.sam, mail=account.mail or "", values=_values_of(account))
|
||||
|
||||
|
||||
def _record_details_failure(user: SessionUser, ip: str, reason: str, **detail) -> None:
|
||||
audit.record_failure(
|
||||
user.key, "profile.update_failed", ip=ip, detail={"reason": reason, **detail}
|
||||
)
|
||||
|
||||
|
||||
def update_details(user: SessionUser, values: dict[str, str], *, ip: str) -> DetailsUpdated:
|
||||
"""Save the changed details of the signed-in user.
|
||||
|
||||
Reads the directory afresh, checks the values that differ and writes only those, with the
|
||||
audit entry in the same transaction (AD-4). Raises DetailsInvalid, DetailsNotAccepted,
|
||||
DirectoryUnavailable or OutcomeUnknown. Must be called outside an open transaction.
|
||||
"""
|
||||
directory = get_directory()
|
||||
try:
|
||||
account = _read(user)
|
||||
except DirectoryUnavailable:
|
||||
_record_details_failure(user, ip, "unavailable")
|
||||
raise
|
||||
|
||||
current = _values_of(account)
|
||||
read = Details(username=account.sam, mail=account.mail or "", values=current)
|
||||
cleaned, errors = check_details(values, current)
|
||||
if errors:
|
||||
raise DetailsInvalid(errors, read)
|
||||
changed = tuple(name for name in DETAIL_FIELDS if cleaned[name] != current[name])
|
||||
if not changed:
|
||||
return DetailsUpdated((), account)
|
||||
|
||||
changes = {DETAIL_ATTRIBUTES[name][0]: cleaned[name] for name in changed}
|
||||
updated = replace(account, **{DETAIL_ATTRIBUTES[name][1]: cleaned[name] for name in changed})
|
||||
changed_in_directory = False
|
||||
step = "directory"
|
||||
try:
|
||||
with transaction.atomic():
|
||||
directory.update_attributes(user.sam, changes)
|
||||
changed_in_directory = True
|
||||
step = "audit"
|
||||
audit.record(user.key, "profile.updated", ip=ip, detail={"fields": list(changed)})
|
||||
step = "commit"
|
||||
except ValidationFailed as exc:
|
||||
_record_details_failure(user, ip, "refused")
|
||||
raise DetailsNotAccepted(read) from exc
|
||||
except DirectoryUnavailable:
|
||||
_record_details_failure(user, ip, "unavailable")
|
||||
raise
|
||||
except Forbidden as exc: # the service account lacks the right: the user sees "not possible"
|
||||
logger.error(
|
||||
"The directory refused a profile write: user=%s ip=%s fields=%s",
|
||||
user.key,
|
||||
ip,
|
||||
",".join(changed),
|
||||
)
|
||||
_record_details_failure(user, ip, "forbidden")
|
||||
raise DirectoryUnavailable("The directory refused the write") from exc
|
||||
except Exception as exc:
|
||||
if changed_in_directory:
|
||||
# AD-4: the directory changed but the portal's part did not commit.
|
||||
logger.error(
|
||||
"Profile details changed in the directory but the portal did not record them: "
|
||||
"user=%s ip=%s time=%s fields=%s step=%s error=%s",
|
||||
user.key,
|
||||
ip,
|
||||
timezone.now().isoformat(),
|
||||
",".join(changed),
|
||||
step,
|
||||
type(exc).__name__,
|
||||
)
|
||||
try:
|
||||
_record_details_failure(user, ip, "unclear", step=step, fields=list(changed))
|
||||
except Exception:
|
||||
logger.error("The failure entry for %s could not be written", user.key)
|
||||
raise OutcomeUnknown("The details were changed but not recorded") from exc
|
||||
_record_details_failure(user, ip, "error")
|
||||
raise
|
||||
return DetailsUpdated(changed, updated)
|
||||
|
||||
@@ -3,6 +3,7 @@ from django.urls import path
|
||||
from . import views
|
||||
|
||||
urlpatterns = [
|
||||
path("profile", views.profile, name="profile"),
|
||||
path("password", views.password, name="password"),
|
||||
path("password/done", views.password_done, name="password-done"),
|
||||
]
|
||||
|
||||
+53
-1
@@ -10,12 +10,13 @@ from django.views.decorators.debug import sensitive_post_parameters
|
||||
from django.views.decorators.http import require_http_methods
|
||||
|
||||
from portal.accounts.decorators import login_required
|
||||
from portal.accounts.identity import remember
|
||||
from portal.accounts.throttle import Throttled
|
||||
from portal.adapters.directory import DirectoryUnavailable
|
||||
from portal.adapters.registry import get_directory
|
||||
|
||||
from . import services
|
||||
from .forms import PasswordChangeForm
|
||||
from .forms import DETAIL_FIELDS, DetailsForm, PasswordChangeForm
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -107,3 +108,54 @@ def password_done(request):
|
||||
if flag is None: # opened directly: nothing to report
|
||||
return redirect("password")
|
||||
return render(request, "profile/password_done.html", {"mail": flag["mail"]})
|
||||
|
||||
|
||||
# -- profile details (FR-9) -------------------------------------------------------------------
|
||||
|
||||
NOTICE_FLAG = "profile_notice" # "saved" or "unchanged", shown once after the redirect
|
||||
|
||||
|
||||
def _details_page(request, form, details, *, status=200, notice=None):
|
||||
context = {
|
||||
"form": form,
|
||||
"details": details,
|
||||
"notice": notice,
|
||||
"fields": [
|
||||
{"field": form[name], "label": label, "max_length": max_length}
|
||||
for name, (label, max_length, _required) in DETAIL_FIELDS.items()
|
||||
],
|
||||
}
|
||||
return render(request, "profile/details.html", context, status=status)
|
||||
|
||||
|
||||
@login_required
|
||||
@require_http_methods(["GET", "POST"])
|
||||
def profile(request):
|
||||
user = request.portal_user.user # the session user only: nothing is read from the request
|
||||
if request.method == "GET":
|
||||
try:
|
||||
details = services.load_details(user)
|
||||
except DirectoryUnavailable:
|
||||
return _unavailable(request)
|
||||
notice = request.session.pop(NOTICE_FLAG, None)
|
||||
return _details_page(request, DetailsForm(initial=details.values), details, notice=notice)
|
||||
|
||||
form = DetailsForm(request.POST)
|
||||
form.is_valid() # trims; the checks need the directory value and run in the service
|
||||
try:
|
||||
result = services.update_details(user, form.cleaned_data, ip=request.client_ip)
|
||||
except services.DetailsInvalid as exc:
|
||||
form.apply_errors(exc.errors)
|
||||
return _details_page(request, form, exc.details, status=400)
|
||||
except services.DetailsNotAccepted as exc:
|
||||
form.add_error(None, _("A value was not accepted. Please check your entries."))
|
||||
return _details_page(request, form, exc.details, status=400)
|
||||
except DirectoryUnavailable:
|
||||
return _unavailable(request)
|
||||
except services.OutcomeUnknown:
|
||||
return render(request, "profile/details_unknown.html", status=500)
|
||||
|
||||
if result.changed:
|
||||
remember(request, result.account) # this session shows the new name at once
|
||||
request.session[NOTICE_FLAG] = "saved" if result.changed else "unchanged"
|
||||
return redirect("profile")
|
||||
|
||||
Reference in New Issue
Block a user