Skip to content

feat: add ability for users to convert their password login type to oauth/github login #8105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 64 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
ba42c17
WIP: working on ability to merge oidc with password auth account
Emyrk Jun 13, 2023
a746952
Fix displaying error on login page
Emyrk Jun 14, 2023
bd93ef9
add migration to add a table for tracking oidc state
Emyrk Jun 14, 2023
b2c9496
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 14, 2023
0078629
WIP: add full functionality, but block the upgrade
Emyrk Jun 14, 2023
fc19a8a
Functionality is working, need to polish and write tests. Currently d…
Emyrk Jun 15, 2023
31a7e78
Begin refactoring to rename and make consistent
Emyrk Jun 15, 2023
069d689
Add dbfakes, delete mergestates, and implement the convert
Emyrk Jun 20, 2023
6d6a46e
Add flag to enable this feature
Emyrk Jun 20, 2023
a4961b9
Add unit test
Emyrk Jun 20, 2023
ab94b85
Add FE unit test for audit logS
Emyrk Jun 20, 2023
8390fb8
Conditionally show account setting
Emyrk Jun 20, 2023
9027c2a
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 20, 2023
b082e49
Make gen
Emyrk Jun 20, 2023
ecf3789
Linting
Emyrk Jun 20, 2023
95ff11a
Remove test lint
Emyrk Jun 20, 2023
6e69f90
Bump migration number
Emyrk Jun 20, 2023
8c2fc33
Swagger annotations
Emyrk Jun 20, 2023
cf19d8c
Swagger annotations
Emyrk Jun 21, 2023
6947021
golden file update
Emyrk Jun 21, 2023
27bf518
Add format tags
Emyrk Jun 21, 2023
3c8ee52
Fix audit log mistake
Emyrk Jun 21, 2023
b4bfc29
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 21, 2023
bc506c1
Make gen
Emyrk Jun 21, 2023
4ea859c
Fix audit log on login failure
Emyrk Jun 21, 2023
847dc64
fixup! Fix audit log on login failure
Emyrk Jun 21, 2023
59e8924
Bump migration
Emyrk Jun 21, 2023
368fa2b
Make gen and add test fixtures
Emyrk Jun 21, 2023
9cfb69c
Add from_login_type
Emyrk Jun 21, 2023
ddabcf7
Extract oauth convert into a helper function
Emyrk Jun 22, 2023
57b3605
chore: add boolean to auth methods to know if the deployment supports…
Emyrk Jun 22, 2023
7aedfbc
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 26, 2023
2be844f
Add back missing code from merge
Emyrk Jun 26, 2023
fde4908
Linting
Emyrk Jun 26, 2023
810e996
Fix merge error
Emyrk Jun 26, 2023
f3bce86
Move error message to route
Emyrk Jun 26, 2023
a2510a2
Rename state_string to state
Emyrk Jun 26, 2023
48db46c
Fix js unit tests
Emyrk Jun 28, 2023
c2aea70
Fix migration fixture
Emyrk Jun 28, 2023
6294fb5
feat(site): add change OIDC UI (#8182)
BrunoQuaresma Jun 28, 2023
68977f5
create a new route for user login type
Emyrk Jun 28, 2023
c8905b6
New route for login type
Emyrk Jun 28, 2023
6ae96c9
Switch state from being stored in the db to a jwt in a cookie
Emyrk Jun 28, 2023
28d2aa9
Fix audit logging changes
Emyrk Jun 28, 2023
92c50cf
Fix audit logs
Emyrk Jun 28, 2023
48f6d30
Linting
Emyrk Jun 28, 2023
af411be
Include mock of new api route
Emyrk Jun 28, 2023
1f2aef9
Do not double commit audit logs
Emyrk Jun 28, 2023
1260546
fmt
Emyrk Jun 28, 2023
31f48a2
Fix js mock rest endpoint
Emyrk Jun 28, 2023
6d7a63f
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 28, 2023
fa76cde
Bump migration
Emyrk Jun 29, 2023
e670557
Make gen
Emyrk Jun 29, 2023
949e687
PR feedback
Emyrk Jun 29, 2023
28e1156
Merge remote-tracking branch 'origin/main' into stevenmasley/merge_oi…
Emyrk Jun 29, 2023
4cee518
Linting
Emyrk Jun 29, 2023
159f12c
Rename Oauth
Emyrk Jun 29, 2023
c9b8fe6
PR Feedback
Emyrk Jun 29, 2023
2ffe5db
Swagger fix
Emyrk Jun 29, 2023
b6d7de5
Make experiment for better tracking
Emyrk Jun 29, 2023
e8d7ec9
experiment make gen
Emyrk Jun 29, 2023
d67e0c7
Update golden files
Emyrk Jun 29, 2023
b124ff4
mock convert endpoint
Emyrk Jun 29, 2023
f393e44
Mock the redirect
Emyrk Jun 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
golden file update
  • Loading branch information
Emyrk committed Jun 21, 2023
commit 694702118025e63017e44aa32c6693aa960b04c2
4 changes: 4 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ networking:
# directly in the database.
# (default: <unset>, type: bool)
disablePasswordAuth: false
# If enabled, users can switch from password based authentication to oauth based
# authentication by logging into an oidc account with the same email address.
# (default: <unset>, type: bool)
enableOauthAuthConversion: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to drop this if that is the conclusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bpmct bpmct Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# The interval in which coderd should be checking the status of workspace proxies.
# (default: 1m0s, type: duration)
proxyHealthInterval: 1m0s
Expand Down
81 changes: 81 additions & 0 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 71 additions & 0 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 51 additions & 0 deletions docs/api/authorization.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,57 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \

To perform this operation, you must be authenticated. [Learn more](authentication.md).

## Convert user from password to oauth authentication

### Code samples

```shell
# Example request using curl
curl -X POST http://coder-server:8080/api/v2/users/convert-login \
-H 'Content-Type: application/json' \
-H 'Accept: application/json' \
-H 'Coder-Session-Token: API_KEY'
```

`POST /users/convert-login`

> Body parameter

```json
{
"email": "user@example.com",
"password": "string",
"to_login_type": "password"
}
```

### Parameters

| Name | In | Type | Required | Description |
| ------ | ---- | ---------------------------------------------------------------------- | -------- | --------------- |
| `body` | body | [codersdk.ConvertLoginRequest](schemas.md#codersdkconvertloginrequest) | true | Convert request |

### Example responses

> 201 Response

```json
{
"expires_at": "string",
"state_string": "string",
"to_login_type": "password",
"user_id": "string"
}
```

### Responses

| Status | Meaning | Description | Schema |
| ------ | ------------------------------------------------------------ | ----------- | ------------------------------------------------------------------------------ |
| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.OauthConversionResponse](schemas.md#codersdkoauthconversionresponse) |

To perform this operation, you must be authenticated. [Learn more](authentication.md).

## Log in user

### Code samples
Expand Down
38 changes: 38 additions & 0 deletions docs/api/schemas.md
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,24 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in
| `autostart` |
| `autostop` |

## codersdk.ConvertLoginRequest

```json
{
"email": "user@example.com",
"password": "string",
"to_login_type": "password"
}
```

### Properties

| Name | Type | Required | Restrictions | Description |
| --------------- | ---------------------------------------- | -------- | ------------ | ---------------------------------------------- |
| `email` | string | true | | |
| `password` | string | true | | |
| `to_login_type` | [codersdk.LoginType](#codersdklogintype) | true | | To login type is the login type to convert to. |

## codersdk.CreateFirstUserRequest

```json
Expand Down Expand Up @@ -2992,6 +3010,26 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in
| `sign_in_text` | string | false | | |
| `username_field` | string | false | | |

## codersdk.OauthConversionResponse

```json
{
"expires_at": "string",
"state_string": "string",
"to_login_type": "password",
"user_id": "string"
}
```

### Properties

| Name | Type | Required | Restrictions | Description |
| --------------- | ---------------------------------------- | -------- | ------------ | ----------- |
| `expires_at` | string | false | | |
| `state_string` | string | false | | |
| `to_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | |
| `user_id` | string | false | | |

## codersdk.Organization

```json
Expand Down