From c0cc67f01ac1b1598e21e131f3ba763bc97a72e8 Mon Sep 17 00:00:00 2001
From: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date: Thu, 27 Oct 2022 09:12:50 +0200
Subject: [PATCH] auth_oidc: do not attempt to generate one's own client
 credentials (#70749)

    these credentials must be issued to authentic by the OIDC provider,
    see for instance https://datatracker.ietf.org/doc/html/rfc6749#section-2.2
---
 .../commands/oidc-register-issuer.py          | 14 ++++++-----
 .../migrations/0001_initial.py                |  6 ++---
 src/authentic2_auth_oidc/models.py            |  5 ++--
 src/authentic2_auth_oidc/utils.py             |  4 +++-
 tests/test_auth_oidc.py                       | 18 ++++++++++++--
 tests/test_commands.py                        | 24 +++++++++++++++++--
 tests/test_manager_authenticators.py          |  9 +++++--
 7 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/src/authentic2_auth_oidc/management/commands/oidc-register-issuer.py b/src/authentic2_auth_oidc/management/commands/oidc-register-issuer.py
index 8d1e9977b..daef5c9e8 100644
--- a/src/authentic2_auth_oidc/management/commands/oidc-register-issuer.py
+++ b/src/authentic2_auth_oidc/management/commands/oidc-register-issuer.py
@@ -60,6 +60,12 @@ class Command(BaseCommand):
         name = options['name']
         openid_configuration = options.get('openid_configuration')
         issuer = options.get('issuer')
+        client_id = options.get('client_id')
+        if not client_id:
+            raise CommandError('Client identifier must be specified')
+        client_secret = options.get('client_secret')
+        if not client_secret:
+            raise CommandError('Client secret must be specified')
         if openid_configuration:
             with open(openid_configuration) as fd:
                 openid_configuration = json.load(fd)
@@ -70,6 +76,8 @@ class Command(BaseCommand):
                     ou = OrganizationalUnit.objects.get(slug=options['ou_slug'])
                 provider = register_issuer(
                     name,
+                    client_id,
+                    client_secret,
                     issuer=issuer,
                     openid_configuration=openid_configuration,
                     verify=not options['no_verify'],
@@ -87,12 +95,6 @@ class Command(BaseCommand):
         except ValidationError as e:
             provider.delete()
             raise CommandError(e)
-        client_id = options.get('client_id')
-        if client_id:
-            provider.client_id = client_id
-        client_secret = options.get('client_secret')
-        if client_secret:
-            provider.client_secret = client_secret
         scope = options.get('scope')
         if scope is not None:
             provider.scopes = ' '.join(filter(None, options['scope']))
diff --git a/src/authentic2_auth_oidc/migrations/0001_initial.py b/src/authentic2_auth_oidc/migrations/0001_initial.py
index 03eadbd98..cf961e4a8 100644
--- a/src/authentic2_auth_oidc/migrations/0001_initial.py
+++ b/src/authentic2_auth_oidc/migrations/0001_initial.py
@@ -1,5 +1,3 @@
-import uuid
-
 import django.contrib.postgres.fields.jsonb
 from django.conf import settings
 from django.db import migrations, models
@@ -70,10 +68,10 @@ class Migration(migrations.Migration):
                     'issuer',
                     models.CharField(unique=True, max_length=256, verbose_name='issuer', db_index=True),
                 ),
-                ('client_id', models.CharField(default=uuid.uuid4, max_length=128, verbose_name='client id')),
+                ('client_id', models.CharField(max_length=128, verbose_name='client id')),
                 (
                     'client_secret',
-                    models.CharField(default=uuid.uuid4, max_length=128, verbose_name='client secret'),
+                    models.CharField(max_length=128, verbose_name='client secret'),
                 ),
                 (
                     'authorization_endpoint',
diff --git a/src/authentic2_auth_oidc/models.py b/src/authentic2_auth_oidc/models.py
index 37a23987d..bc15e2328 100644
--- a/src/authentic2_auth_oidc/models.py
+++ b/src/authentic2_auth_oidc/models.py
@@ -15,7 +15,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import json
-import uuid
 
 from django.conf import settings
 from django.contrib.postgres.fields import JSONField
@@ -81,8 +80,8 @@ class OIDCProvider(BaseAuthenticator):
     ]
 
     issuer = models.CharField(max_length=256, verbose_name=_('issuer'), db_index=True)
-    client_id = models.CharField(max_length=128, default=uuid.uuid4, verbose_name=_('client id'))
-    client_secret = models.CharField(max_length=128, default=uuid.uuid4, verbose_name=_('client secret'))
+    client_id = models.CharField(max_length=128, verbose_name=_('client id'))
+    client_secret = models.CharField(max_length=128, verbose_name=_('client secret'))
     # endpoints
     authorization_endpoint = models.URLField(max_length=128, verbose_name=_('authorization endpoint'))
     token_endpoint = models.URLField(max_length=128, verbose_name=_('token endpoint'))
diff --git a/src/authentic2_auth_oidc/utils.py b/src/authentic2_auth_oidc/utils.py
index 809f52607..e7ac8fb2e 100644
--- a/src/authentic2_auth_oidc/utils.py
+++ b/src/authentic2_auth_oidc/utils.py
@@ -186,7 +186,9 @@ def check_https(url):
     return urllib.parse.urlparse(url).scheme == 'https'
 
 
-def register_issuer(name, issuer=None, openid_configuration=None, verify=True, timeout=None, ou=None):
+def register_issuer(
+    name, client_id, client_secret, issuer=None, openid_configuration=None, verify=True, timeout=None, ou=None
+):
     from . import models
 
     if issuer and not openid_configuration:
diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py
index d2b973c57..5551c4a8c 100644
--- a/tests/test_auth_oidc.py
+++ b/tests/test_auth_oidc.py
@@ -161,6 +161,8 @@ def make_oidc_provider(
     idtoken_algo=OIDCProvider._meta.get_field('idtoken_algo').default,
     jwkset=None,
     claims_parameter_supported=False,
+    client_id='abc',
+    client_secret='def',
 ):
     slug = slug or name.lower()
     issuer = issuer or ('https://%s.example.com' % slug)
@@ -169,6 +171,8 @@ def make_oidc_provider(
         ou=get_default_ou(),
         name=name,
         slug=slug,
+        client_id=client_id,
+        client_secret=client_secret,
         enabled=True,
         issuer=issuer,
         authorization_endpoint='%s/authorize' % issuer,
@@ -843,12 +847,22 @@ def test_register_issuer(db, app, caplog, oidc_provider_jwkset):
         return oidc_provider_jwkset.export()
 
     with HTTMock(jwks_mock):
-        register_issuer(name='test_issuer', issuer='https://default.issuer', openid_configuration=oidc_conf)
+        register_issuer(
+            name='test_issuer',
+            client_id='abc',
+            client_secret='def',
+            issuer='https://default.issuer',
+            openid_configuration=oidc_conf,
+        )
 
     oidc_conf['id_token_signing_alg_values_supported'] = ['HS256']
     with HTTMock(jwks_mock):
         register_issuer(
-            name='test_issuer_hmac_only', issuer='https://hmac_only.issuer', openid_configuration=oidc_conf
+            name='test_issuer_hmac_only',
+            client_id='ghi',
+            client_secret='jkl',
+            issuer='https://hmac_only.issuer',
+            openid_configuration=oidc_conf,
         )
 
 
diff --git a/tests/test_commands.py b/tests/test_commands.py
index fba3c68c9..6868ac578 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -274,7 +274,16 @@ def test_oidc_register_issuer(db, tmpdir, monkeypatch):
     with oidc_conf_f.open() as f:
         oidc_conf = json.load(f)
 
-    def register_issuer(name, issuer=None, openid_configuration=None, verify=True, timeout=None, ou=None):
+    def register_issuer(
+        name,
+        client_id,
+        client_secret,
+        issuer=None,
+        openid_configuration=None,
+        verify=True,
+        timeout=None,
+        ou=None,
+    ):
         ou = OrganizationalUnit.objects.get(default=True)
         # skipping jwkset retrieval in mocked function
         jwkset = JWKSet()
@@ -283,6 +292,8 @@ def test_oidc_register_issuer(db, tmpdir, monkeypatch):
         return OIDCProvider.objects.create(
             name=name,
             slug='test',
+            client_id='abc',
+            client_secret='def',
             enabled=True,
             ou=ou,
             issuer=issuer,
@@ -299,7 +310,16 @@ def test_oidc_register_issuer(db, tmpdir, monkeypatch):
 
     oidc_conf = py.path.local(__file__).dirpath('openid_configuration.json').strpath
     call_command(
-        'oidc-register-issuer', '--openid-configuration', oidc_conf, '--issuer', 'issuer', 'somename'
+        'oidc-register-issuer',
+        '--openid-configuration',
+        oidc_conf,
+        '--issuer',
+        'issuer',
+        '--client-id',
+        'auie',
+        '--client-secret',
+        'tsrn',
+        'somename',
     )
 
     provider = OIDCProvider.objects.get(name='somename')
diff --git a/tests/test_manager_authenticators.py b/tests/test_manager_authenticators.py
index 0c59e8da7..34feee3c0 100644
--- a/tests/test_manager_authenticators.py
+++ b/tests/test_manager_authenticators.py
@@ -166,6 +166,8 @@ def test_authenticators_oidc(app, superuser, ou1, ou2):
 
     resp = resp.click('Edit')
     assert 'enabled' not in resp.form.fields
+    assert resp.pyquery('input#id_client_id').val() == ''
+    assert resp.pyquery('input#id_client_secret').val() == ''
     resp.form['ou'] = ou1.pk
     resp.form['issuer'] = 'https://oidc.example.com'
     resp.form['scopes'] = 'profile email'
@@ -176,6 +178,8 @@ def test_authenticators_oidc(app, superuser, ou1, ou2):
     resp.form['idtoken_algo'] = 2
     resp.form['button_label'] = 'Test'
     resp.form['button_description'] = 'test'
+    resp.form['client_id'] = 'auie'
+    resp.form['client_secret'] = 'tsrn'
     resp = resp.form.submit().follow()
     assert_event('authenticator.edit', user=superuser, session=app.session)
 
@@ -195,8 +199,9 @@ def test_authenticators_oidc(app, superuser, ou1, ou2):
     resp = resp.click('Journal')
     assert 'enable' in resp.text
     assert (
-        'edit (ou, issuer, scopes, strategy, button_label, idtoken_algo, token_endpoint, '
-        'userinfo_endpoint, button_description, authorization_endpoint)' in resp.text
+        'edit (ou, issuer, scopes, strategy, client_id, button_label, idtoken_algo, '
+        'client_secret, token_endpoint, userinfo_endpoint, button_description, authorization_endpoint)'
+        in resp.text
     )
     assert 'creation' in resp.text
 
-- 
2.37.2

