[feature] overhaul the oidc system #961

theSuess · 2022-11-05T12:16:29Z

Since I'm very interested in this topic, I've built a draft to implement my feedback on #763, #917 and #309

This is by no means final, and very much intended as a base for further discussions on these issues.

It works by intercepting the first OIDC callback of a user and returns a small page for customizing the username if no matching user is found. The text field is prefilled with the preferred_username claim.

It also performs validation and checks the DB for colliding usernames

The biggest open topic IMHO is finding a way to migrate existing OIDC setups to the new ExternalID field. I honestly have no idea what would be the best approach here

tsmethurst

This is looking pretty tasty IMO. Got a few comments about clarification and stuff. Overall, this looks like it's going in a great direction. I'm glad you bit the bullet and just jumped into it, it's always a nice surprise to get a really good PR like this :)

internal/api/client/auth/callback.go

tsmethurst · 2022-11-05T12:33:10Z

internal/oidc/claims.go

+}
+
+func init() {
+	gob.Register(&Claims{})


Could you explain this? I'm not sure what it does :P

Sure: the way I implemented the finalization page, the claim temporarily gets stored in the session. Since the session package uses gob to serialize the struct, we need to register it as serializable. I'm open to cleaner approaches - am not too happy with this as well

cmd/gotosocial/action/admin/account/account.go

tsmethurst · 2022-11-05T12:37:44Z

internal/cache/user.go

@@ -87,6 +87,11 @@ func (c *UserCache) GetByEmail(email string) (*gtsmodel.User, bool) {
 	return c.cache.GetBy("email", email)
 }

+// GetByExternalID attempts to fetch a user from the cache by its external id, you will receive a copy for thread-safety
+func (c *UserCache) GetByExternalID(id string) (*gtsmodel.User, bool) {
+	return c.cache.GetBy("external_id", id)


Does this need to be registered with the cache as a lookup @NyaaaWhatsUpDoc ?

theSuess · 2022-11-10T16:34:23Z

I had an idea for a graceful migration path:

Introduce a config flag to try matching the email to existing users
If a user already exists for a login request but does not have the externalID set, ask the user if they want to link the account
If yes, populate the externalID field
If not, ask the user to provide a new username

What do you think about this?

theSuess · 2022-11-12T09:10:00Z

I had an idea for a graceful migration path:

* Introduce a config flag to try matching the email to existing users

* If a user already exists for a login request but does _not_ have the externalID set, ask the user if they want to link the account

* If yes, populate the externalID field

* If not, ask the user to provide a new username

What do you think about this?

I've now implemented this (without the step of asking for a new username for now).

With this, we have a migration path for servers already using OIDC

tsmethurst · 2022-11-12T11:39:13Z

Heya, apologies for being slow to respond to this, it's been rather a hectic week :')

This looks good to me so far! At least, I don't see any glaring issues, but will have to try it out on gts.superseriousbusiness.org (which uses oidc) to see how it works :)

One thing you missed is adding the new config flag to examples/config.yaml, and also updating the documentation here https://github.com/superseriousbusiness/gotosocial/blob/main/docs/configuration/oidc.md.

Once those things are done, I think you can mark this as ready for review and we can have a proper look through it.

Thank you, this is great work!

theSuess · 2022-11-12T14:54:36Z

No problem, I'm just happy to hack on something not work-related 😁

I've added the docs and example config. I always seem to forget at least one place when adding new config options :D

Also marked this PR as ready 🤞

tsmethurst · 2022-11-13T13:04:16Z

This looks good to me :) @NyaaaWhatsUpDoc do you also wanna review or nah?

tsmethurst · 2022-11-20T17:06:49Z

hey @theSuess , can you merge main into this or rebase to resolve the merge conflicts? then i'll look at it afresh

theSuess · 2022-11-21T15:47:46Z

Rebased and tested

web/template/finalize.tmpl

NyaaaWhatsUpDoc · 2022-11-22T08:26:11Z

I'll take a look at this today too :) sorry I managed to miss your notifications on this @tsmethurst

tsmethurst · 2022-11-22T08:35:20Z

No worries pal :)

docs/configuration/oidc.md

NyaaaWhatsUpDoc · 2022-11-23T22:12:44Z

I have started to review this, but I have ran out of spoons for the evening, and it is late. I shall get back to it tomorrow evening most likely :)

tsmethurst · 2022-11-26T11:26:46Z

i suggest we merge this once 0.6.0 is out, so we have time to mess about with it :)

[edit] Also sorry for the constant need for rebases dominik 😬

internal/api/client/auth/callback.go

this allows for more flexible username handling and prevents account takeover using old email addresses

NyaaaWhatsUpDoc · 2022-12-01T19:58:35Z

internal/api/client/auth/callback.go

+func (m *Module) FinalizePOSTHandler(c *gin.Context) {
+	s := sessions.Default(c)
+
+	form := &extraInfo{}


Could we get some code comments in here? It's quite a heavy unseparated function block

I've sprinkled in some comments - let me know if something is still unclear

NyaaaWhatsUpDoc · 2022-12-01T20:03:12Z

internal/api/client/auth/callback.go

 	if errWithCode != nil {
 		m.clearSession(s)
 		api.ErrorHandler(c, errWithCode, m.processor.InstanceGet)
 		return
 	}
-
+	if user == nil {


Again this is quite a heavy unseparated function block. I know this is quite nitpicky I'm just a bit of a stickler for readability!

Also added some comments here. We could also split this up into its own function, but not sure if that would increase readability by much

theSuess force-pushed the oidc-overhaul branch from cb7f9bd to 9f3be8b Compare November 5, 2022 12:19

tsmethurst reviewed Nov 5, 2022

View reviewed changes

theSuess force-pushed the oidc-overhaul branch from 9f3be8b to f06bed0 Compare November 12, 2022 09:08

theSuess force-pushed the oidc-overhaul branch 2 times, most recently from 98c72e5 to bc87fe7 Compare November 12, 2022 09:28

theSuess marked this pull request as ready for review November 12, 2022 14:54

theSuess force-pushed the oidc-overhaul branch 2 times, most recently from 69681f8 to b382797 Compare November 21, 2022 15:45

theSuess force-pushed the oidc-overhaul branch from b382797 to 0593356 Compare November 21, 2022 16:01

dwmunster reviewed Nov 22, 2022

View reviewed changes

web/template/finalize.tmpl Outdated Show resolved Hide resolved

theSuess force-pushed the oidc-overhaul branch from 0593356 to dfe79ea Compare November 22, 2022 07:09

tsmethurst mentioned this pull request Nov 23, 2022

[feature] Custom Admin OIDC Groups #1121

Closed

NyaaaWhatsUpDoc reviewed Nov 23, 2022

View reviewed changes

docs/configuration/oidc.md Outdated Show resolved Hide resolved

theSuess force-pushed the oidc-overhaul branch from dfe79ea to 13f63f3 Compare November 25, 2022 13:05

royalfork reviewed Nov 27, 2022

View reviewed changes

internal/api/client/auth/callback.go Show resolved Hide resolved

theSuess added 4 commits November 30, 2022 17:22

[feature] overhaul the oidc system

12812bb

this allows for more flexible username handling and prevents account takeover using old email addresses

[feature] add migration path for old OIDC users

68a5ffe

[feature] nicer error reporting for users

0bea4c8

[docs] document the new OIDC flow

0c4caab

theSuess force-pushed the oidc-overhaul branch from 13f63f3 to 0c4caab Compare November 30, 2022 16:24

[fix] return early on oidc error

a90db30

NyaaaWhatsUpDoc reviewed Dec 1, 2022

View reviewed changes

[docs]: add comments on the finalization logic

9c80b3f

tsmethurst merged commit 199b685 into superseriousbusiness:main Dec 6, 2022

This was referenced Dec 8, 2022

[feature] OIDC: support identity providers without name in id_token #309

Open

[feature] Use the sub claim for OIDC Accounts #917

Closed

[feature] Make fields for OIDC username/display name etc configurable #763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] overhaul the oidc system #961

[feature] overhaul the oidc system #961

theSuess commented Nov 5, 2022

tsmethurst left a comment

tsmethurst Nov 5, 2022

theSuess Nov 5, 2022

tsmethurst Nov 5, 2022

theSuess commented Nov 10, 2022

theSuess commented Nov 12, 2022

tsmethurst commented Nov 12, 2022 •

edited

Loading

theSuess commented Nov 12, 2022

tsmethurst commented Nov 13, 2022

tsmethurst commented Nov 20, 2022

theSuess commented Nov 21, 2022

NyaaaWhatsUpDoc commented Nov 22, 2022

tsmethurst commented Nov 22, 2022

NyaaaWhatsUpDoc commented Nov 23, 2022

tsmethurst commented Nov 26, 2022 •

edited

Loading

NyaaaWhatsUpDoc Dec 1, 2022

theSuess Dec 2, 2022

NyaaaWhatsUpDoc Dec 1, 2022

theSuess Dec 2, 2022

[feature] overhaul the oidc system #961

[feature] overhaul the oidc system #961

Conversation

theSuess commented Nov 5, 2022

tsmethurst left a comment

Choose a reason for hiding this comment

tsmethurst Nov 5, 2022

Choose a reason for hiding this comment

theSuess Nov 5, 2022

Choose a reason for hiding this comment

tsmethurst Nov 5, 2022

Choose a reason for hiding this comment

theSuess commented Nov 10, 2022

theSuess commented Nov 12, 2022

tsmethurst commented Nov 12, 2022 • edited Loading

theSuess commented Nov 12, 2022

tsmethurst commented Nov 13, 2022

tsmethurst commented Nov 20, 2022

theSuess commented Nov 21, 2022

NyaaaWhatsUpDoc commented Nov 22, 2022

tsmethurst commented Nov 22, 2022

NyaaaWhatsUpDoc commented Nov 23, 2022

tsmethurst commented Nov 26, 2022 • edited Loading

NyaaaWhatsUpDoc Dec 1, 2022

Choose a reason for hiding this comment

theSuess Dec 2, 2022

Choose a reason for hiding this comment

NyaaaWhatsUpDoc Dec 1, 2022

Choose a reason for hiding this comment

theSuess Dec 2, 2022

Choose a reason for hiding this comment

tsmethurst commented Nov 12, 2022 •

edited

Loading

tsmethurst commented Nov 26, 2022 •

edited

Loading