providers/oauth2: OpenID conformance (goauthentik#4758)

* don't open inspector by default when debug is enabled Signed-off-by: Jens Langhammer <[email protected]> * encode error in fragment when using hybrid grant_type Signed-off-by: Jens Langhammer <[email protected]> * require nonce for all response_types that get an id_token from the authorization endpoint Signed-off-by: Jens Langhammer <[email protected]> * don't set empty family_name Signed-off-by: Jens Langhammer <[email protected]> * only set at_hash when response has token Signed-off-by: Jens Langhammer <[email protected]> * cleaner way to get login time Signed-off-by: Jens Langhammer <[email protected]> * remove authentication requirement from authentication flow Signed-off-by: Jens Langhammer <[email protected]> * use wrapper Signed-off-by: Jens Langhammer <[email protected]> * fix auth_time not being handled correctly Signed-off-by: Jens Langhammer <[email protected]> * minor cleanup Signed-off-by: Jens Langhammer <[email protected]> * add test files Signed-off-by: Jens Langhammer <[email protected]> * fix tests Signed-off-by: Jens Langhammer <[email protected]> * remove USER_LOGIN_AUTHENTICATED Signed-off-by: Jens Langhammer <[email protected]> * rework prompt=login handling Signed-off-by: Jens Langhammer <[email protected]> * also set last login uid for max_age check to prevent double login when max_age and prompt=login is set Signed-off-by: Jens Langhammer <[email protected]> --------- Signed-off-by: Jens Langhammer <[email protected]>
anarock · Feb 23, 2023 · 80f4fcc · 80f4fcc
1 parent c6a14fa
commit 80f4fcc
Show file tree

Hide file tree

Showing 22 changed files with 255 additions and 65 deletions.
diff --git a/authentik/api/tests/test_auth.py b/authentik/api/tests/test_auth.py
@@ -4,6 +4,7 @@
 
 from django.conf import settings
 from django.test import TestCase
+from django.utils import timezone
 from rest_framework.exceptions import AuthenticationFailed
 
 from authentik.api.authentication import bearer_auth
@@ -68,6 +69,7 @@ def test_jwt_valid(self):
             user=create_test_admin_user(),
             provider=provider,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope=SCOPE_AUTHENTIK_API,
             _id_token=json.dumps({}),
         )
@@ -82,6 +84,7 @@ def test_jwt_missing_scope(self):
             user=create_test_admin_user(),
             provider=provider,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="",
             _id_token=json.dumps({}),
         )

diff --git a/authentik/core/api/applications.py b/authentik/core/api/applications.py
@@ -37,7 +37,6 @@
 from authentik.policies.api.exec import PolicyTestResultSerializer
 from authentik.policies.engine import PolicyEngine
 from authentik.policies.types import PolicyResult
-from authentik.stages.user_login.stage import USER_LOGIN_AUTHENTICATED
 
 LOGGER = get_logger()
 
@@ -186,10 +185,6 @@ def list(self, request: Request) -> Response:
         if superuser_full_list and request.user.is_superuser:
             return super().list(request)
 
-        # To prevent the user from having to double login when prompt is set to login
-        # and the user has just signed it. This session variable is set in the UserLoginStage
-        # and is (quite hackily) removed from the session in applications's API's List method
-        self.request.session.pop(USER_LOGIN_AUTHENTICATED, None)
         queryset = self._filter_queryset_for_list(self.get_queryset())
         self.paginate_queryset(queryset)
 

diff --git a/authentik/flows/stage.py b/authentik/flows/stage.py
@@ -231,7 +231,7 @@ def __init__(self, executor: "FlowExecutorView", error_message: Optional[str] =
     def get_challenge(self, *args, **kwargs) -> Challenge:
         return AccessDeniedChallenge(
             data={
-                "error_message": self.error_message or "Unknown error",
+                "error_message": str(self.error_message or "Unknown error"),
                 "type": ChallengeTypes.NATIVE.value,
                 "component": "ak-stage-access-denied",
             }

diff --git a/authentik/providers/oauth2/api/providers.py b/authentik/providers/oauth2/api/providers.py
@@ -1,5 +1,6 @@
 """OAuth2Provider API Views"""
 from django.urls import reverse
+from django.utils import timezone
 from drf_spectacular.utils import OpenApiResponse, extend_schema
 from rest_framework.decorators import action
 from rest_framework.fields import CharField
@@ -153,6 +154,7 @@ def preview_user(self, request: Request, pk: int) -> Response:
                 user=request.user,
                 provider=provider,
                 _scope=" ".join(scope_names),
+                auth_time=timezone.now(),
             ),
             request,
         )

diff --git a/authentik/providers/oauth2/errors.py b/authentik/providers/oauth2/errors.py
@@ -174,10 +174,12 @@ def create_uri(self) -> str:
 
         # See:
         # http://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthError
-        hash_or_question = "#" if self.grant_type == GrantTypes.IMPLICIT else "?"
+        fragment_or_query = (
+            "#" if self.grant_type in [GrantTypes.IMPLICIT, GrantTypes.HYBRID] else "?"
+        )
 
         uri = (
-            f"{self.redirect_uri}{hash_or_question}error="
+            f"{self.redirect_uri}{fragment_or_query}error="
             f"{self.error}&error_description={description}"
         )
 

diff --git a/authentik/providers/oauth2/id_token.py b/authentik/providers/oauth2/id_token.py
@@ -110,12 +110,11 @@ def new(
         # Convert datetimes into timestamps.
         now = timezone.now()
         id_token.iat = int(now.timestamp())
+        id_token.auth_time = int(token.auth_time.timestamp())
 
         # We use the timestamp of the user's last successful login (EventAction.LOGIN) for auth_time
         auth_event = get_login_event(request)
         if auth_event:
-            auth_time = auth_event.created
-            id_token.auth_time = int(auth_time.timestamp())
             # Also check which method was used for authentication
             method = auth_event.context.get(PLAN_CONTEXT_METHOD, "")
             method_args = auth_event.context.get(PLAN_CONTEXT_METHOD_ARGS, {})

diff --git a/...ders/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py b/...ders/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py
@@ -0,0 +1,40 @@
+# Generated by Django 4.1.7 on 2023-02-22 22:23
+
+import django.utils.timezone
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("authentik_providers_oauth2", "0014_alter_refreshtoken_options_and_more"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="accesstoken",
+            name="auth_time",
+            field=models.DateTimeField(
+                default=django.utils.timezone.now,
+                verbose_name="Authentication time",
+            ),
+            preserve_default=False,
+        ),
+        migrations.AddField(
+            model_name="authorizationcode",
+            name="auth_time",
+            field=models.DateTimeField(
+                default=django.utils.timezone.now,
+                verbose_name="Authentication time",
+            ),
+            preserve_default=False,
+        ),
+        migrations.AddField(
+            model_name="refreshtoken",
+            name="auth_time",
+            field=models.DateTimeField(
+                default=django.utils.timezone.now,
+                verbose_name="Authentication time",
+            ),
+            preserve_default=False,
+        ),
+    ]
diff --git a/authentik/providers/oauth2/models.py b/authentik/providers/oauth2/models.py
@@ -226,7 +226,7 @@ def jwt_key(self) -> tuple[str | PRIVATE_KEY_TYPES, str]:
     def get_issuer(self, request: HttpRequest) -> Optional[str]:
         """Get issuer, based on request"""
         if self.issuer_mode == IssuerMode.GLOBAL:
-            return request.build_absolute_uri("/")
+            return request.build_absolute_uri(reverse("authentik_core:root-redirect"))
         try:
             url = reverse(
                 "authentik_providers_oauth2:provider-root",
@@ -282,6 +282,7 @@ class BaseGrantModel(models.Model):
     user = models.ForeignKey(User, verbose_name=_("User"), on_delete=models.CASCADE)
     revoked = models.BooleanField(default=False)
     _scope = models.TextField(default="", verbose_name=_("Scopes"))
+    auth_time = models.DateTimeField(verbose_name="Authentication time")
 
     @property
     def scope(self) -> list[str]:

diff --git a/authentik/providers/oauth2/tests/test_authorize.py b/authentik/providers/oauth2/tests/test_authorize.py
@@ -204,6 +204,7 @@ def test_response_type(self):
                 "redirect_uri": "http://local.invalid/Foo",
                 "scope": "openid",
                 "state": "foo",
+                "nonce": generate_id(),
             },
         )
         self.assertEqual(
@@ -325,6 +326,7 @@ def test_full_implicit(self):
                     "state": state,
                     "scope": "openid",
                     "redirect_uri": "http://localhost",
+                    "nonce": generate_id(),
                 },
             )
             response = self.client.get(
@@ -378,6 +380,7 @@ def test_full_form_post_id_token(self):
                 "state": state,
                 "scope": "openid",
                 "redirect_uri": "http://localhost",
+                "nonce": generate_id(),
             },
         )
         response = self.client.get(

diff --git a/authentik/providers/oauth2/tests/test_introspect.py b/authentik/providers/oauth2/tests/test_introspect.py
@@ -4,6 +4,7 @@
 from dataclasses import asdict
 
 from django.urls import reverse
+from django.utils import timezone
 
 from authentik.core.models import Application
 from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow
@@ -41,6 +42,7 @@ def test_introspect_refresh(self):
             provider=self.provider,
             user=self.user,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="openid user profile",
             _id_token=json.dumps(
                 asdict(
@@ -72,6 +74,7 @@ def test_introspect_access(self):
             provider=self.provider,
             user=self.user,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="openid user profile",
             _id_token=json.dumps(
                 asdict(

diff --git a/authentik/providers/oauth2/tests/test_revoke.py b/authentik/providers/oauth2/tests/test_revoke.py
@@ -4,6 +4,7 @@
 from dataclasses import asdict
 
 from django.urls import reverse
+from django.utils import timezone
 
 from authentik.core.models import Application
 from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow
@@ -40,6 +41,7 @@ def test_revoke_refresh(self):
             provider=self.provider,
             user=self.user,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="openid user profile",
             _id_token=json.dumps(
                 asdict(
@@ -62,6 +64,7 @@ def test_revoke_access(self):
             provider=self.provider,
             user=self.user,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="openid user profile",
             _id_token=json.dumps(
                 asdict(

diff --git a/authentik/providers/oauth2/tests/test_token.py b/authentik/providers/oauth2/tests/test_token.py
@@ -4,6 +4,7 @@
 
 from django.test import RequestFactory
 from django.urls import reverse
+from django.utils import timezone
 
 from authentik.core.models import Application
 from authentik.core.tests.utils import create_test_admin_user, create_test_flow
@@ -45,7 +46,9 @@ def test_request_auth_code(self):
         )
         header = b64encode(f"{provider.client_id}:{provider.client_secret}".encode()).decode()
         user = create_test_admin_user()
-        code = AuthorizationCode.objects.create(code="foobar", provider=provider, user=user)
+        code = AuthorizationCode.objects.create(
+            code="foobar", provider=provider, user=user, auth_time=timezone.now()
+        )
         request = self.factory.post(
             "/",
             data={
@@ -99,6 +102,7 @@ def test_request_refresh_token(self):
             provider=provider,
             user=user,
             token=generate_id(),
+            auth_time=timezone.now(),
         )
         request = self.factory.post(
             "/",
@@ -127,7 +131,9 @@ def test_auth_code_view(self):
         self.app.save()
         header = b64encode(f"{provider.client_id}:{provider.client_secret}".encode()).decode()
         user = create_test_admin_user()
-        code = AuthorizationCode.objects.create(code="foobar", provider=provider, user=user)
+        code = AuthorizationCode.objects.create(
+            code="foobar", provider=provider, user=user, auth_time=timezone.now()
+        )
         response = self.client.post(
             reverse("authentik_providers_oauth2:token"),
             data={
@@ -173,6 +179,7 @@ def test_refresh_token_view(self):
             user=user,
             token=generate_id(),
             _id_token=dumps({}),
+            auth_time=timezone.now(),
         )
         response = self.client.post(
             reverse("authentik_providers_oauth2:token"),
@@ -221,6 +228,7 @@ def test_refresh_token_view_invalid_origin(self):
             user=user,
             token=generate_id(),
             _id_token=dumps({}),
+            auth_time=timezone.now(),
         )
         response = self.client.post(
             reverse("authentik_providers_oauth2:token"),
@@ -271,6 +279,7 @@ def test_refresh_token_revoke(self):
             user=user,
             token=generate_id(),
             _id_token=dumps({}),
+            auth_time=timezone.now(),
         )
         # Create initial refresh token
         response = self.client.post(

diff --git a/authentik/providers/oauth2/tests/test_userinfo.py b/authentik/providers/oauth2/tests/test_userinfo.py
@@ -3,6 +3,7 @@
 from dataclasses import asdict
 
 from django.urls import reverse
+from django.utils import timezone
 
 from authentik.blueprints.tests import apply_blueprint
 from authentik.core.models import Application
@@ -37,6 +38,7 @@ def setUp(self) -> None:
             provider=self.provider,
             user=self.user,
             token=generate_id(),
+            auth_time=timezone.now(),
             _scope="openid user profile",
             _id_token=json.dumps(
                 asdict(
@@ -56,7 +58,6 @@ def test_userinfo_normal(self):
             {
                 "name": self.user.name,
                 "given_name": self.user.name,
-                "family_name": "",
                 "preferred_username": self.user.name,
                 "nickname": self.user.name,
                 "groups": [group.name for group in self.user.ak_groups.all()],
@@ -79,7 +80,6 @@ def test_userinfo_invalid_scope(self):
             {
                 "name": self.user.name,
                 "given_name": self.user.name,
-                "family_name": "",
                 "preferred_username": self.user.name,
                 "nickname": self.user.name,
                 "groups": [group.name for group in self.user.ak_groups.all()],