Do users need to be able to do this? What if we only allowed admins to change the state manually?

User's trigger this. I took the idea from here #4505 (comment)

Essentially linking users to their OIDC account is tricky, as emails can match even if they are different people. Eg maybe an account was made with builtin called paul@coder.com. Paul leaves the company. The next Paul is hired and now it's difficult to know in the code if the Paul from OIDC is the same Paul from the builtin.

This resolves that by making it a user triggered action. The user goes to their Account > Settings. Hits "Convert to OIDC" and goes through the OIDC flow.

We generate a state string linking the OIDC login to the "Convert" request. So we know it is the same person as they were able to provide the password for the builtin AND provide the OIDC auth.

Can we encode something in the state instead? We already sign the value, I believe.

Err, if we don't we can. We have a global signing key that we use anyways.

As for the why: I've always found it sketchy to insert any OAuth state into the database. It allows for an unauthenticated user to directly insert into the DB.

I did think about signing state, but all Oauth 2.0 specs I read said it should be a non-guessable random string. I don't see a security risk with signing a random string and making it a JWT for example. We should have expiration and user_id as apart of the state string, so there is information that needs to be encoded.

So some IDP's do have a maximum length state param. These values are different everywhere

From https://auth0.com/docs/secure/attack-protection/state-parameters

The allowed length for state is not unlimited. If you get the error 414 Request-URI Too Large, try a smaller value.

In practice, the smallest length I am finding is 255. It's not well documented anywhere.

Bunq: Max length 255

Github, Gitlab, Google, etc do not document this max length. The common language is that the state string carries some CSRF protection.

I did a test JWT and it was 371 characters long.

In summary, I agree a JWT would be simpler as it requires no database interactions to pull out the info. And I honestly prefer to go the JWT method. My only fear is that someone using coder might hit a state string too large error with their IDP, and there would not be any recourse.

On the "Sketchy to insert OAuth sate into the database" because of unauthenticated users. In this case, only authenticated users can generate this convert to OIDC state string. An unauthenticated user cannot trigger this codepath.

I have code that makes sure only 1 state string per user exists in the DB.

So in this particular case, we avoid your worries.

If you still think we should go JWT instead, I can make the switch. I just honestly don't know what state string limitations exist, and I spent over an hour trying to find information without any luck besides errors people hit in random github issues on random projects.

This should clear the stored hashed password too

Done and added a unit test.

@bpmct do we need to add a new option for this? Is there any problem with this being default instead? It feels bad to add a new config option just for this.

Happy to drop this if that is the conclusion.

Actually I think we should keep this atm.

This flag is hidden because we need to follow up this PR. See "Decisions to be made" section in the description.

I want to get this PR in as it's been hung up for awhile. Those decisions can come after this. Then we can drop this flag.

Maybe I should make it an experiment. Ideally the follow up is quick.

Many of the "decisions to be made" feel like breaking changes if we merge this and users start using it. I know it's been hung up for a while, but what is the benefit of merging this PR?

Let's move forward with this:

After OIDC convert, should we log out the user?

Yes

Allow builtin login after conversion?

No. Users can only log in with their login type

If email mismatch, should we update the users email? Should we make a new account? What do we do?

We should update the user's email and expect that ignore_changes is used in templates to protect resources from destruction.

Should we allow convert back to password auth?

Yes, but the password has to be reset. The password should also be cleared when the login type is changed from builtin.

I'm fine with removing enableOauthAuthConversion and making always true and non-configurable. As an aside, we work with many customers whose security teams do not allow built-in accounts of any type. We should eventually add a flag for that (creating/converting/etc). I will make an issue for it.

I made this an experiment. Just in review terms, adding these adds more re-reviewing. If it's an experiment, I can do quick follow ups. As long as it's an experiment, it will not just idle and get lost imo.