Rebase webauthn / passkey PR onto main #2153

Javex · 2023-10-28T00:37:37Z

This is a rebase of #1618 in which @dave-atx added webauthn support. All credit to them for doing the hard work of adding this feature. Due to the significant changes since this PR on the main branch, there's quite a few things that have moved around, but it's mostly still recognisably the feature dave-atx implemented.

Notable changes I made from that PR:

Added untranslated id_ID for webauthn since that language wasn't there back then
Moved from duo-labs module to go-webauthn (already discussed in the original PR)
Added support for allowCredentials and excludeCredentials. Note that the webauthn spec documents a slight privacy and security risk for allowCredentials, but I don't think miniflux needs to worry about this narrow case.
Fixed a few issues that made the Javascript fail in Firefox
Remove rp.icon which is deprecated and makes Firefox fail if it's present. The current spec doesn't include it any more.

The original description & instructions from the PR still apply, which I'll repeat here (with edits) to make it easier:

Adds a new WEBAUTHN config option to allow WebAuthn to be used instead of a manual password. The config option defaults to true. When enabled, a new "login with passkey" button shows on the login page and an option to register a passkey appears on the settings page.
BASE_URL must be set to http://localhost:8080 (or equivalent) or passkeys will fail.
The ability to remove individual passkeys is still missing

I would also appreciate if somebody could help with the go.mod and go.sum files. I'm still new to Go and not entirely sure if those files are purely PR changes or if I accidentally updated unrelated modules. The rebase was a bit complex so I may have messed things up in the process.

Do you follow the guidelines?

I have tested my changes
I read this document: https://miniflux.app/faq.html#pull-request

Javex · 2023-10-29T05:07:54Z

I've now added the option to manage individual passkeys. It's a bit more involved, because passkeys themselves don't really make it easy so you have to add it yourselves, the protocol doesn't supply user friendly names and being able to delete passkeys based on their random binary data didn't seem super helpful. Would gladly take some tips if you think there's a better structure for the data or if I should organise the code differently.

fguillot

Thank you for taking the time to do this. I was able to test it and that seems to work.

I added few minor comments. Other than that, it would be nice if you can update the man page with the new config option.

I would also appreciate if somebody could help with the go.mod and go.sum files. I'm still new to Go and not entirely sure if those files are purely PR changes or if I accidentally updated unrelated modules. The rebase was a bit complex so I may have messed things up in the process.

It should be fine if you rebase against latest changes in main, and run go mod tidy.

fguillot · 2023-11-02T13:24:38Z

internal/locale/translations/fr_FR.json

+    "page.settings.webauthn.passkey_name": "Passkey Name",
+    "page.settings.webauthn.added_on": "Added On",
+    "page.settings.webauthn.last_seen_on": "Last Used",
+    "page.settings.webauthn.register": "Enregistrer le mot de passe",


The french translation is incorrect: mot de passe is the translation for password. clé d'accès might be more appropriate here.

You can leave the text in English and I will translate later.

Fixed: ceaec0c

I've removed all the French text and left it in English, wasn't sure if you meant just the one.

Note that for all languages, I didn't do any translation: If it looks translated, it's based on the old PR and where I introduced new strings, I didn't translate them, probably best to get native speakers to do that. I can do the German one if you need help there. I took the easy route here so far and just didn't touch it.

fguillot · 2023-11-02T13:26:57Z

internal/model/webauthn.go

@@ -0,0 +1,49 @@
+package model // import "miniflux.app/v2/internal/model"


Minor detail: The license header is missing in the new files.

Added: fbf9aaa

I followed the example and only added it for .go files, but not .js or .html, hope that was right.

fguillot · 2023-11-02T13:27:51Z

internal/storage/webauthn.go

+// attestation_type varchar(255) not null,
+// aaguid bytea,
+// sign_count bigint,
+// clone_warning bool


These comments looks unnecessary

Removed: e4e6f29

fguillot · 2023-11-02T13:30:50Z

internal/storage/webauthn.go

+// handle storage of webauthn credentials
+func (s *Storage) AddWebAuthnCredential(userID int64, handle []byte, credential *webauthn.Credential) error {
+	query := `
+		insert into webauthn_credentials 


Minor detail: it would be nice to have the SQL queries follow the existing naming convention: INSERT INTO webauthn_credentials ...

Fixed in 9611ac0 - is this what you had in mind? Looks better like this, I think :)

fguillot · 2023-11-02T13:36:31Z

template/templates/views/settings.html

@@ -0,0 +1,135 @@
+{{ define "title"}}{{ t "page.settings.title" }}{{ end }}


Looks like this file should not be there after the rebase.

You're right! Totally missed that, thanks! 4c0975c

fguillot · 2023-11-02T13:39:39Z

internal/model/webauthn.go

+import (
+	"database/sql/driver"
+	"encoding/hex"
+	jsonenc "encoding/json"


the jsonenc should not be necessary here?

Huh don't even know where that came from, I assume you meant the "alias" wasn't needed? See 774a479

This was missed in the original rebase and accidentally added in its old path.

Introduce a list of passkeys and options to remove and rename them. Because the native library data structure does not have space for this new metadata, this change introduces a wrapping struct to old on to that data and hence there are quite a few changes to interfaces. It is also quite tricky to identify passkeys just based on the information they give you, so inspired by Github, you are now prompted to create a name for your passkey. Combined with when they were created and last used that should make management easier. The field is intentionally optional if users don't want to enter a name. When registering one they will now be prompted, but can choose to ignore that or enter an empty name. Registration has already succeeded at that point.

Javex · 2023-11-05T09:17:15Z

Thank you for taking the time to do this. I was able to test it and that seems to work.

I added few minor comments. Other than that, it would be nice if you can update the man page with the new config option.

I would also appreciate if somebody could help with the go.mod and go.sum files. I'm still new to Go and not entirely sure if those files are purely PR changes or if I accidentally updated unrelated modules. The rebase was a bit complex so I may have messed things up in the process.

It should be fine if you rebase against latest changes in main, and run go mod tidy.

I hope I did that correctly: 6206738

I've addressed the comments, let me know if this is good or if you'd like me to do something else. Thanks for taking a look.

I also get the feeling we might want to squash this before merging, I messed up quite a bit with the rebases so condensing it down / cleaning it up might make for a nicer history (not sure how important that is to you).

fguillot reviewed Nov 2, 2023

View reviewed changes

dave-atx and others added 22 commits November 5, 2023 18:16

WebAuthn / passkey login support

398f2f2

correct linter issues

bea907d

adding translations for webauthn strings

019d740

correct plurals for webauthn

15dabf3

swallow aborted conditional logins

7fa745c

Rebase fixes

dc54a68

Fix $d to %d in German translation

dada2a3

Correctly decode base64url in JS

a1b211b

Prevent autosubmit in Firefox

7550aa5

Add support for allow & excludeCredentials

05dd1a6

Remove deprecated icon from response (breaks Firefox)

9357c57

Add back uk_UA translations that got lost during rebase

48ee7f5

Add new untranslated strings for id_ID for webauthn

3fdbd07

Remove accidental rebase artefact

4ac28f0

This was missed in the original rebase and accidentally added in its old path.

go mod tidy after rebase

6206738

Leave French texts untranslated

ceaec0c

Add license header

fbf9aaa

Remove jsonenc alias and reference json directly

774a479

Remove unnecessary comments

e4e6f29

Reformat SQL queries

9611ac0

Remove extra file after rebase

4c0975c

Javex force-pushed the webauthn branch from 7dfc986 to 4c0975c Compare November 5, 2023 09:07

fguillot merged commit 62ef8ed into miniflux:main Nov 5, 2023
15 checks passed

Javex deleted the webauthn branch November 5, 2023 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase webauthn / passkey PR onto main #2153

Rebase webauthn / passkey PR onto main #2153

Javex commented Oct 28, 2023

Javex commented Oct 29, 2023

fguillot left a comment

fguillot Nov 2, 2023

Javex Nov 5, 2023

fguillot Nov 2, 2023

Javex Nov 5, 2023

fguillot Nov 2, 2023

Javex Nov 5, 2023

fguillot Nov 2, 2023

Javex Nov 5, 2023

fguillot Nov 2, 2023

Javex Nov 5, 2023

fguillot Nov 2, 2023

Javex Nov 5, 2023

Javex commented Nov 5, 2023

		@@ -0,0 +1,49 @@
		package model // import "miniflux.app/v2/internal/model"

		@@ -0,0 +1,135 @@
		{{ define "title"}}{{ t "page.settings.title" }}{{ end }}

Rebase webauthn / passkey PR onto main #2153

Rebase webauthn / passkey PR onto main #2153

Conversation

Javex commented Oct 28, 2023

Javex commented Oct 29, 2023

fguillot 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Javex commented Nov 5, 2023