81 refactor i uzupełnienie swaggera do metod auth by JanekDr · Pull Request #174 · Solvro/backend-testownik

JanekDr · 2026-03-15T17:27:32Z

Migracja klas: Wszystkie endpointy Solvro/USOS OAuth przeniesione na DRF APIView/AsyncAPIView dla poprawnego generowania schematu. Wszelkie opcje redirect korzystają z parametryzacji docelowych widoków klasowych.
Opisy wejść/wyjść: Dodano dekoratory @extend_schema ze szczegółowym wykazem rzucanych kodów błędów (200, 302, 400, 401, 403, 404) dla każdej ścieżki JWT i OTP.
Bugfix walidacji na OTP: Dodano authentication_classes = [] do publicznych widoków OTP i LoginLinkView. Eliminacja błędnych response 401 Unauthorized dla starych sesji.
Wszystkie endpointy są dostępne w sekcji Authentication w dokumentacji.

…t + move it to authentication section

users/views.py

-            return HttpResponseForbidden()
+                return redirect(add_query_params(redirect_url, {"error": "no_email"}))
+            messages.error(request, "Brak adresu email w profilu użytkownika.")
+            return redirect(redirect_url)


users/views.py

+        auth_login(request, user)
+        return redirect(redirect_url)

-    await async_auth_login(request, user)


users/views.py

+                messages.error(request, f"Twoje konto zostało zablokowane: {user.ban_reason or 'Brak powodu'}")
+                return redirect(redirect_url)
+
+            if not user.is_student_and_not_staff:


users/views.py

+        await async_auth_login(request, user)
+        return redirect(redirect_url)




In general, the safest approach is to ensure that any user-provided redirect URL is normalized and strictly validated before use. That means:

Normalize input (strip whitespace, normalize backslashes, and lowercase schemes/hosts).

Reject protocol-relative (//...) and backslash-prefixed (\\...) URLs.

For non-absolute paths, only allow safe relative URLs.

For absolute URLs, accept only http/https, and only when their origin is in ALLOWED_REDIRECT_ORIGINS, matches the current host, or matches a carefully defined preview pattern.

Fall back to a safe default path when validation fails.

The current helper already aims to do this, but we can tighten and clarify it:

Normalize the input early:

Strip leading/trailing whitespace.

Replace \ with / before parsing, to avoid browser/backslash quirks.

Use a single urlparse call and re-use its result; avoid parsing different string variants.

Treat URLs without a scheme but with a netloc-like prefix (e.g., example.com/path) as unsafe, even though browsers can resolve them.

For relative URLs, rely on Django’s url_has_allowed_host_and_scheme with allowed_hosts={request.get_host(), *parsed allowlist hosts} and require_https as configured.

For absolute URLs, compare normalized origins (scheme://netloc lowercased and stripped of trailing slashes) against ALLOWED_REDIRECT_ORIGINS similarly normalized, and apply the preview regexes to that normalized origin.

Ensure that get_safe_redirect_url can still safely handle the "index" default and whatever URL pattern your code already uses there.

Concretely, we’ll modify:

is_safe_redirect_url to:

Normalize backslashes to forward slashes before any startswith/urlparse operation.

Lowercase the scheme and host in comparisons.

Normalize ALLOWED_REDIRECT_ORIGINS to comparable origin strings.

Use a single urlparse result and reduce duplication.

get_safe_redirect_url stays as is logically, but continues to rely on the improved is_safe_redirect_url; its fallback default remains unchanged to preserve behavior.

No changes are needed to the call site in UsosAuthorizeView; after the sanitizer is strengthened, redirect(redirect_url) will be safe.

users/views.py

…auth

…ttps://github.com/Solvro/backend-testownik into 81-refactor-i-uzupełnienie-swaggera-do-metod-auth

Antoni-Czaplicki · 2026-03-30T15:29:39Z

@JanekDr co myślisz żeby zrobić helper safe_redirect który w sobie sprawdzi czy jest safe i jak coś to nadpisze?

users/views.py

Antoni-Czaplicki · 2026-03-30T15:32:30Z

i pamiętaj też żeby dać Resolve jak już jest komentarz ogarnięty

JanekDr · 2026-03-31T12:46:57Z

@JanekDr co myślisz żeby zrobić helper safe_redirect który w sobie sprawdzi czy jest safe i jak coś to nadpisze?

Myśle ze to dobry pomysł i uszczupli lekko kod w metodach auth dla usosa i solvro

Copilot

Pull request overview

Refactors the authentication-related OAuth and OTP endpoints in users to improve DRF Spectacular (Swagger/OpenAPI) schema generation, while tightening redirect safety checks and documenting request/response variants.

Changes:

Migrates Solvro/USOS OAuth endpoints from function-based views to DRF APIView / adrf async API views.
Expands @extend_schema documentation across JWT, OAuth, and OTP endpoints (params, examples, and response codes).
Updates redirect safety logic and adds tests for DEBUG-dependent HTTP/HTTPS redirect handling; adds initial OAuth flow tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`users/views.py`	New class-based OAuth endpoints, enhanced schema docs, updated redirect safety logic, and public OTP views made explicitly unauthenticated.
`users/urls.py`	Routes updated to point to the new class-based OAuth views.
`users/tests/test_redirect_url.py`	Adds DEBUG=True/False coverage for HTTP vs HTTPS redirect acceptance.
`users/tests/test_oauth.py`	Adds tests asserting callback URL composition and a mocked Solvro authorize flow.

Comments suppressed due to low confidence (1)

users/views.py:574

Same add_query_params + view-name issue in the banned-user jwt=true branch: if redirect_url is the default view name, redirect(add_query_params(...)) can raise NoReverseMatch and return 500 instead of redirecting with an error.

            if request.GET.get("jwt", "false") == "true":
                auth_params = {"error": "user_banned"}
                if user.ban_reason:
                    auth_params["ban_reason"] = user.ban_reason
                return redirect(add_query_params(redirect_url, auth_params))

Copilot · 2026-04-01T18:05:10Z

users/views.py

+                "USOS credentials not configured. USOS_CONSUMER_KEY=%s, USOS_CONSUMER_SECRET=%s",
+                "<set>" if usos_key else "<missing>",
+                "<set>" if usos_secret else "<missing>",
            )
+            return redirect(add_query_params(redirect_url, {"error": "usos_unavailable"}))


redirect_url is allowed to be empty here (missing/blocked redirect param). In that case add_query_params(redirect_url, ...) will produce a string like ?error=..., and django.shortcuts.redirect() will raise NoReverseMatch because it treats it as a view name (no / or .). Consider falling back to a concrete URL/path (e.g., resolved index URL) when redirect_url is empty, or return a 400 response instead of attempting to redirect.

Copilot · 2026-04-01T18:05:10Z

users/views.py

+                if attempt < max_retries - 1:
+                    await sleep(retry_delay)
+                    continue
+
+                return redirect(add_query_params(redirect_url, {"error": "usos_unavailable"}))
+
+        return redirect(add_query_params(redirect_url, {"error": "usos_unavailable"}))


Same issue as above: on the final retry, redirect_url may still be empty, and redirect(add_query_params(redirect_url, ...)) can crash with NoReverseMatch. Handle the empty/invalid redirect case explicitly before redirecting with query params.

Copilot · 2026-04-01T18:05:11Z

users/views.py

+            302: OpenApiResponse(
+                description="Redirect to USOS OAuth provider, or redirect with `?error=usos_unavailable` on failure."
+            ),
+            400: OpenApiResponse(description="Invalid redirect URL."),


The OpenAPI schema advertises a 400 "Invalid redirect URL" response, but this view no longer returns 400 for an invalid redirect value (it gets replaced by the default in get_safe_redirect_url). Either reintroduce an explicit 400 for invalid redirects or remove/adjust the documented 400 so the schema matches runtime behavior.

Suggested change

400: OpenApiResponse(description="Invalid redirect URL."),

Copilot · 2026-04-01T18:05:11Z

users/views.py

+        if not profile.get("email"):
+            logger.error("Solvro user profile missing email. Profile keys: %s", list(profile.keys()))
            if request.GET.get("jwt", "false") == "true":
-                return redirect(add_query_params(redirect_url, {"error": "authorization_failed"}))
-            return HttpResponseForbidden()
+                return redirect(add_query_params(redirect_url, {"error": "no_email"}))
+            messages.error(request, "Brak adresu email w profilu użytkownika.")


In the jwt=true path, if redirect is missing/blocked then redirect_url falls back to the view name index. Calling add_query_params('index', ...) produces a string like index?error=no_email, and redirect() will raise NoReverseMatch (500) because it treats that as a view name. Ensure redirect_url is a real URL/path before appending query params (e.g., resolve the view name first or use a path default like /).

Copilot · 2026-04-01T18:05:11Z

users/tests/test_oauth.py

+from django.test import TestCase, override_settings
+from django.urls import reverse
+from rest_framework.test import APIClient
+
+from users.models import User
+
+
+@override_settings(DEBUG=True)
+class SolvroOAuthTests(TestCase):
+    def setUp(self):
+        self.client = APIClient()


These are API endpoint tests but they use django.test.TestCase + APIClient. Elsewhere in this repo, API tests consistently use rest_framework.test.APITestCase (e.g., users/tests/test_jwt.py, users/tests/test_guest.py), which provides better DRF integration and consistent helpers. Consider switching this test class to APITestCase for consistency.

Suggested change

from django.test import TestCase, override_settings

from django.urls import reverse

from rest_framework.test import APIClient

from users.models import User

@override_settings(DEBUG=True)

class SolvroOAuthTests(TestCase):

def setUp(self):

self.client = APIClient()

from django.test import override_settings

from django.urls import reverse

from rest_framework.test import APITestCase

from users.models import User

@override_settings(DEBUG=True)

class SolvroOAuthTests(APITestCase):

JanekDr added 6 commits March 10, 2026 21:37

refactor: change redirecting function based views to class based views

7120f3b

feat: add 404 usos errors to docs

7ea1913

feat: complete otp login docs

611f7e3

feat: add JWT token in docs

d9e9b08

fix: disable JWT validation on public OTP/login-link endpoints

8af391b

refactor: change response message for 403 error in login-otp/ endpoin…

4009f65

…t + move it to authentication section

JanekDr self-assigned this Mar 15, 2026

JanekDr requested a review from MoonPrincess06 as a code owner March 15, 2026 17:27

JanekDr linked an issue Mar 15, 2026 that may be closed by this pull request

Refactor i uzupełnienie swaggera do metod auth #81

Open

pull-request-size bot added the size/XL label Mar 15, 2026

github-advanced-security bot found potential problems Mar 15, 2026

View reviewed changes

JanekDr added 2 commits March 15, 2026 19:18

fix: secure open redirect vulnerability using Django validation

e42ac68

fix: restore strict origin and scheme checks for redirect URLs

132e597

MoonPrincess06 requested changes Mar 17, 2026

View reviewed changes

users/views.py Outdated Show resolved Hide resolved

users/views.py Outdated Show resolved Hide resolved

JanekDr added 2 commits March 18, 2026 12:07

refactor: replace lambda expression with def

0812ebf

test: add solvro oauth tests

07929e3

JanekDr requested a review from MoonPrincess06 March 18, 2026 14:28

MoonPrincess06 requested changes Mar 22, 2026

View reviewed changes

users/views.py Show resolved Hide resolved

JanekDr added 4 commits March 22, 2026 21:26

fix: prevent HTTP redirects in is_safe_redirect_url on production

f937aa2

Merge branch 'dev' into 81-refactor-i-uzupełnienie-swaggera-do-metod-…

3719003

…auth

test: override DEBUG settings to allow HTTP redirects in OAuth tests

d9015fb

Merge branch '81-refactor-i-uzupełnienie-swaggera-do-metod-auth' of h…

88f9e4c

…ttps://github.com/Solvro/backend-testownik into 81-refactor-i-uzupełnienie-swaggera-do-metod-auth

JanekDr requested a review from MoonPrincess06 March 29, 2026 18:13

Antoni-Czaplicki reviewed Mar 30, 2026

View reviewed changes

users/views.py Show resolved Hide resolved

refactor: add safe redirect helper to ouath views

a411b65

Antoni-Czaplicki requested a review from Copilot April 1, 2026 17:59

Copilot started reviewing on behalf of Antoni-Czaplicki April 1, 2026 17:59 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

@@ -224,50 +224,68 @@
                 if url == "admin:index":
                     return True
-                url = str(url).strip()
+                # Normalize the URL: strip whitespace and normalize backslashes to avoid
+                # browser-specific backslash handling quirks.
+                url = str(url).strip().replace("\\", "/")
-                if url.startswith("//") or url.startswith("\\\\"):
+                # Reject protocol-relative URLs like //example.com/...
+                if url.startswith("//"):
                     return False
+                # Allow simple relative paths (handled by Django as local redirects).
                 if url.startswith("/"):
                     return True
+                try:
+                    parsed = urlparse(url)
+                except Exception:
+                    return False
+                # Build the set of allowed hosts from configured origins and the current request.
                 allowed_hosts = {urlparse(origin).netloc for origin in ALLOWED_REDIRECT_ORIGINS}
+                allowed_hosts = {h.lower() for h in allowed_hosts if h}
                 if request:
-                    allowed_hosts.add(request.get_host())
+                    allowed_hosts.add(request.get_host().lower())
+                # Let Django perform its own safety checks (handles schemes, hosts, etc.).
                 is_django_safe = url_has_allowed_host_and_scheme(
                     url,
-                    allowed_hosts=allowed_hosts,
+                    allowed_hosts=allowed_hosts or None,
                     require_https=not getattr(settings, "DEBUG", False),
                 )
-                try:
-                    parsed = urlparse(url)
-                    if parsed.scheme not in ("http", "https"):
-                        return False
+                # Only http/https are ever acceptable for absolute URLs.
+                if parsed.scheme.lower() not in ("http", "https"):
+                    return False
-                    if not parsed.netloc:
-                        return False
+                if not parsed.netloc:
+                    # Absolute-looking URL without a netloc (e.g. malformed); treat as unsafe.
+                    return False
-                    url_origin = f"{parsed.scheme}://{parsed.netloc}".rstrip("/")
+                url_origin = f"{parsed.scheme.lower()}://{parsed.netloc.lower()}".rstrip("/")
-                    if is_django_safe:
-                        if url_origin in ALLOWED_REDIRECT_ORIGINS:
-                            return True
-                        if request and parsed.netloc == request.get_host():
-                            return True
+                if is_django_safe:
+                    # Normalize configured origins for comparison.
+                    normalized_allowed_origins = {
+                        urlparse(origin).scheme.lower() + "://" + urlparse(origin).netloc.lower()
+                        for origin in ALLOWED_REDIRECT_ORIGINS
+                        if urlparse(origin).scheme and urlparse(origin).netloc
+                    }
+                    normalized_allowed_origins = {o.rstrip("/") for o in normalized_allowed_origins}
-                    if ALLOW_PREVIEW_ENVIRONMENTS:
-                        for regex in PREVIEW_ORIGIN_REGEXES:
-                            if re.match(regex, url_origin):
-                                return True
+                    if url_origin in normalized_allowed_origins:
+                        return True
+                    if request and parsed.netloc.lower() == request.get_host().lower():
+                        return True
-                    return False
-                except Exception:
-                    return False
+                if ALLOW_PREVIEW_ENVIRONMENTS:
+                    for regex in PREVIEW_ORIGIN_REGEXES:
+                        if re.match(regex, url_origin):
+                            return True
+                return False
             def get_safe_redirect_url(url: str, request=None, default="index") -> str:
                 if not url:
                     return default

		await async_auth_login(request, user)
		return redirect(redirect_url)

Conversation

JanekDr commented Mar 15, 2026

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Uh oh!

Uh oh!

Uh oh!

Antoni-Czaplicki commented Mar 30, 2026

Uh oh!

Uh oh!

Antoni-Czaplicki commented Mar 30, 2026

Uh oh!

JanekDr commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants