Clarify custom credential model usage #26

jmichalicek · 2024-09-17T15:47:49Z

This PR is to resolve #21

Moved AbstractWebAuthnAttestation.credential field the concrete WebAuthnAttestation model
Added a check on AbstractWebAuthnAttestation to ensure that there is a credential field
Updated README.md with instructions on how to implement custom models.

The credential field was moved because it always needs to be specified by the user. When implementing their own credential model, a custom attestation model with a relation back to their custom credential has to be specified. If implementing only an attestation model then the credential field still needs to be overridden or the related_name would cause a conflict due to having two models with the same related_name pointing at WebAuthnAttestation.

A check was added to the AbstractWebAuthnAttestation model to ensure that it has a credential. We could take that a step further and, if it exists, ensure that it is a OneToOneField relating back to an AbstractWebAuthnCredential, but a user doesn't technically need subclass that as long as they put the correct fields and methods on their model (although mypy will complain if they use that don't subclass properly).

Locally I added a swapped_models app with a handful of models. I'll keep it around for reference. That could be good to make use of when writing a test suite (something I just started looking into doing with hatch the other day).

jmichalicek · 2024-09-17T15:51:47Z

src/django_otp_webauthn/models.py

@@ -64,14 +65,6 @@ class Format(models.TextChoices):
        FIDO_U2F = "fido-u2f", "fido-u2f"
        NONE = "none", "none"

-    credential = models.OneToOneField(


SInce this got moved and __str__() references self.credential, we may also want to specify just the type annotation for that field on this model if we want to run mypy on it regularly. I'm not going to fight with it right now, though, unless you'd rather solve for it immediately.

I don't fancy picking a fight with the type checking system either. I'll leave that up to someone else as a future improvement.

Stormheg

Great docs @jmichalicek! Some minor points 👍

Stormheg · 2024-09-18T12:00:44Z

README.md

+    """
+    A WebAuthn credential associated with a specific site.
+    """
+
+    site = models.ForeignKey(Site, on_delete=models.CASCADE)


Suggestion: I don't think a illustrative example of a customization is necessary, we can just pass here and keep it short.

Stormheg · 2024-09-18T12:02:13Z

README.md

+The `AbstractWebAuthnCredential` model creates an index with a name which includes the concrete model's name with `_sha256_idx` appended to the end. If this combination is longer than 30 characters then you will also need to override the index on your credential model to ensure an appropriate length for the index name.
+
+
+```python
+class MyCredentiialModelWithALongName(AbstractWebAuthnCredential):
+
+    class Meta:
+        indexes = [
+            models.Index(fields=["credential_id_sha256"], name="mycredential_id_sha256_idx"),
+        ]
+```


Praise: good callout! I ran into this already when I experimented with the suggestions I made in #21

Stormheg · 2024-09-18T12:04:19Z

src/django_otp_webauthn/models.py

+                        "missing 'credential' field",
+                        hint=f"Add a field named 'credential' as a OneToOneField relating to {get_credential_model_string()}.",
+                        obj=cls,
+                        id="otp_webauthn.E001",


Suggestion: can we make this refer to a constant in checks.py? I like to keep an overview of what error codes we have defined in one place.

Yep, can't believe I didn't think to do that in the first place.

@Stormheg Somehow I had totally missed that you already had a checks.py with constants in it for current errors. Is there a scheme you are using for choosing the error codes?

I haven't thought about it much. Looking at it now, it looks like I grouped similar checks together

ERR_NO_RP_ID = "otp_webauthn.E010"
ERR_NO_RP_NAME = "otp_webauthn.E011"
ERR_UNSUPPORTED_COSE_ALGORITHM = "otp_webauthn.E020"
ERR_NO_COSE_ALGORITHMS_CONFIGURED = "otp_webauthn.E021"
...

I'd just add to the list of constants. The next available code following this scheme would be otp_webauthn.E050

Sounds good, I'll have this ready to go at some point this weekend. Not much work to do, just gotta find time around family, etc.

Stormheg · 2024-09-18T12:06:05Z

src/django_otp_webauthn/models.py

@@ -64,14 +65,6 @@ class Format(models.TextChoices):
        FIDO_U2F = "fido-u2f", "fido-u2f"
        NONE = "none", "none"

-    credential = models.OneToOneField(


I don't fancy picking a fight with the type checking system either. I'll leave that up to someone else as a future improvement.

Stormheg · 2024-09-18T12:07:20Z

README.md

+
+
+```python
+class MyCredentiialModelWithALongName(AbstractWebAuthnCredential):


Suggested change

class MyCredentiialModelWithALongName(AbstractWebAuthnCredential):

class MyCredentialModelWithALongName(AbstractWebAuthnCredential):

Stormheg · 2024-09-18T12:10:47Z

README.md

@@ -198,6 +199,54 @@ If you are exclusively using Passkeys as a secondary verification step, you don'

 9. That's it! You should now see a "Register Passkey" button on your logged-in user template. Clicking this button will start the registration process. After registration, you should see a "Login using a Passkey" button on your login page. Clicking this button will prompt you to use your Passkey to authenticate. Or if your browser supports it, you will be prompted to use your Passkey when you focus the username field.

+## Using custom credential and attestation models.
+
+Django OTP WebAuthn provides its own models for credentials and attestations for those credentials. If these do not suit your needs you can also provide your own models. When using a custom credential model them you *must* use a custom attestation model as well.


Thought: we should explain better what attestations do to the uniniated, but that is a topic for the docs which have yet to be written...

Stormheg · 2024-09-18T12:16:28Z

Locally I added a swapped_models app with a handful of models. I'll keep it around for reference. That could be good to make use of when writing a test suite (something I just started looking into doing with hatch the other day).

@jmichalicek I have already started work on a testsuite. I'll make sure to commit and push the few test cases I have along with some CI soon. Would love help with improving test coverage if you're up for that.

jmichalicek · 2024-09-20T11:40:40Z

Locally I added a swapped_models app with a handful of models. I'll keep it around for reference. That could be good to make use of when writing a test suite (something I just started looking into doing with hatch the other day).

@jmichalicek I have already started work on a testsuite. I'll make sure to commit and push the few test cases I have along with some CI soon. Would love help with improving test coverage if you're up for that.

I'd be happy to help with writing tests. I spend a lot of time writing tests and advocating for improving test coverage and testing techniques (or having them at all, in some cases) as part of my job. The same goes for any other related stuff - formatter and linter configuration and enforcement, fighting with mypy as has already come up, etc.

* Moved `AbstractWebAuthnAttestation.credential` field the concrete `WebAUthnAttestation` model * Added a check on `AbstractWebAuthnAttestation` to ensure that there is a `credential` field * Updated README.md with instructions on how to implement custom models.

jmichalicek · 2024-10-21T11:07:56Z

@Stormheg just a small bump for this PR

Stormheg

Dropped of my radar – LGTM ✅ ✅ 👍

Stormheg · 2024-10-22T15:02:36Z

I'm going to spend some more on the test suite and get something going before making a release. Expect new things and a release at the end of the week.

Thanks again for the help 👍

jmichalicek · 2024-10-26T01:41:34Z

I'm going to spend some more on the test suite and get something going before making a release. Expect new things and a release at the end of the week.

Thanks again for the help 👍

Glad to help. This is very useful to me. I also love helping open source stuff and while helping the big guys is fun, helping smaller projects is more important. Everyone wants to help the big ones. Managing a small one is hard (not that managing a big one isn't, just a different hard). I have a couple of my own with a few users and, honestly, I fear them taking off in any way.

jmichalicek commented Sep 17, 2024

View reviewed changes

jmichalicek force-pushed the swappable-models-fix branch from 2ed63c4 to 8698218 Compare September 17, 2024 16:03

Stormheg reviewed Sep 18, 2024

View reviewed changes

jmichalicek added 2 commits September 30, 2024 07:34

custom credential model docs cleanup

ec0deb2

jmichalicek force-pushed the swappable-models-fix branch from 8698218 to ec0deb2 Compare September 30, 2024 11:34

jmichalicek requested a review from Stormheg September 30, 2024 11:35

Stormheg approved these changes Oct 22, 2024

View reviewed changes

Stormheg merged commit 7ac9679 into Stormbase:main Oct 22, 2024

Stormheg added a commit that referenced this pull request Oct 22, 2024

Add changelog entry for #26

ea473f3

jmichalicek deleted the swappable-models-fix branch October 26, 2024 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify custom credential model usage #26

Clarify custom credential model usage #26

jmichalicek commented Sep 17, 2024 •

edited

Loading

jmichalicek Sep 17, 2024

Stormheg Sep 18, 2024

Stormheg left a comment

Stormheg Sep 18, 2024

Stormheg Sep 18, 2024

Stormheg Sep 18, 2024

jmichalicek Sep 20, 2024

jmichalicek Sep 23, 2024

Stormheg Sep 27, 2024

jmichalicek Sep 27, 2024

Stormheg Sep 18, 2024

Stormheg Sep 18, 2024

Stormheg Sep 18, 2024

Stormheg commented Sep 18, 2024

jmichalicek commented Sep 20, 2024

jmichalicek commented Oct 21, 2024

Stormheg left a comment

Stormheg commented Oct 22, 2024

jmichalicek commented Oct 26, 2024



		```python
		class MyCredentiialModelWithALongName(AbstractWebAuthnCredential):

	class MyCredentiialModelWithALongName(AbstractWebAuthnCredential):
	class MyCredentialModelWithALongName(AbstractWebAuthnCredential):

Clarify custom credential model usage #26

Clarify custom credential model usage #26

Conversation

jmichalicek commented Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stormheg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stormheg commented Sep 18, 2024

jmichalicek commented Sep 20, 2024

jmichalicek commented Oct 21, 2024

Stormheg left a comment

Choose a reason for hiding this comment

Stormheg commented Oct 22, 2024

jmichalicek commented Oct 26, 2024

jmichalicek commented Sep 17, 2024 •

edited

Loading