Skip to content

Commit 51f17b1

Browse files
kylecarbskylecarbs
authored
fix: allow disabling all password auth even if owner (coder#6193)
* fix: allow disabling all password auth even if owner Removes any and all ability to auth with a password. * Hide create user if password auth is disabled
1 parent 41ae01d commit 51f17b1

File tree

7 files changed

+116
-42
lines changed

7 files changed

+116
-42
lines changed

‎coderd/userauth.go

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/coder/coder/coderd/database/dbauthz"
2424
"github.com/coder/coder/coderd/httpapi"
2525
"github.com/coder/coder/coderd/httpmw"
26-
"github.com/coder/coder/coderd/rbac"
2726
"github.com/coder/coder/coderd/userpassword"
2827
"github.com/coder/coder/codersdk"
2928
)
@@ -90,19 +89,10 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
9089
// If password authentication is disabled and the user does not have the
9190
// owner role, block the request.
9291
if api.DeploymentConfig.DisablePasswordAuth.Value {
93-
permitted := false
94-
for _, role := range user.RBACRoles {
95-
if role == rbac.RoleOwner() {
96-
permitted = true
97-
break
98-
}
99-
}
100-
if !permitted {
101-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
102-
Message: "Password authentication is disabled. Only administrators can sign in with password authentication.",
103-
})
104-
return
105-
}
92+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
93+
Message: "Password authentication is disabled.",
94+
})
95+
return
10696
}
10797

10898
if user.LoginType != database.LoginTypePassword {

‎coderd/users.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,15 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) {
299299
return
300300
}
301301

302+
// If password auth is disabled, don't allow new users to be
303+
// created with a password!
304+
if api.DeploymentConfig.DisablePasswordAuth.Value {
305+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
306+
Message: "You cannot manually provision new users with password authentication disabled!",
307+
})
308+
return
309+
}
310+
302311
// TODO: @emyrk Authorize the organization create if the createUser will do that.
303312

304313
_, err := api.Database.GetUserByEmailOrUsername(ctx, database.GetUserByEmailOrUsernameParams{

‎coderd/users_test.go

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,6 @@ func TestPostLogin(t *testing.T) {
187187
t.Parallel()
188188

189189
dc := coderdtest.DeploymentConfig(t)
190-
dc.DisablePasswordAuth.Value = true
191190
client := coderdtest.New(t, &coderdtest.Options{
192191
DeploymentConfig: dc,
193192
})
@@ -207,6 +206,8 @@ func TestPostLogin(t *testing.T) {
207206
})
208207
require.NoError(t, err)
209208

209+
dc.DisablePasswordAuth.Value = true
210+
210211
userClient := codersdk.New(client.URL)
211212
_, err = userClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
212213
Email: user.Email,
@@ -217,20 +218,6 @@ func TestPostLogin(t *testing.T) {
217218
require.ErrorAs(t, err, &apiErr)
218219
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
219220
require.Contains(t, apiErr.Message, "Password authentication is disabled")
220-
221-
// Promote the user account to an owner.
222-
_, err = client.UpdateUserRoles(ctx, user.ID.String(), codersdk.UpdateRoles{
223-
Roles: []string{rbac.RoleOwner(), rbac.RoleMember()},
224-
})
225-
require.NoError(t, err)
226-
227-
// Login with the user account.
228-
res, err := userClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
229-
Email: user.Email,
230-
Password: password,
231-
})
232-
require.NoError(t, err)
233-
require.NotEmpty(t, res.SessionToken)
234221
})
235222

236223
t.Run("Success", func(t *testing.T) {

‎site/src/components/SignInForm/SignInForm.stories.tsx

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,26 @@ WithOIDC.args = {
116116
},
117117
}
118118

119+
export const WithOIDCWithoutPassword = Template.bind({})
120+
WithOIDCWithoutPassword.args = {
121+
...SignedOut.args,
122+
authMethods: {
123+
password: { enabled: false },
124+
github: { enabled: false },
125+
oidc: { enabled: true, signInText: "", iconUrl: "" },
126+
},
127+
}
128+
129+
export const WithoutAny = Template.bind({})
130+
WithoutAny.args = {
131+
...SignedOut.args,
132+
authMethods: {
133+
password: { enabled: false },
134+
github: { enabled: false },
135+
oidc: { enabled: false, signInText: "", iconUrl: "" },
136+
},
137+
}
138+
119139
export const WithGithubAndOIDC = Template.bind({})
120140
WithGithubAndOIDC.args = {
121141
...SignedOut.args,

‎site/src/components/SignInForm/SignInForm.tsx

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { OAuthSignInForm } from "./OAuthSignInForm"
99
import { BuiltInAuthFormValues } from "./SignInForm.types"
1010
import Button from "@material-ui/core/Button"
1111
import EmailIcon from "@material-ui/icons/EmailOutlined"
12+
import { AlertBanner } from "components/AlertBanner/AlertBanner"
1213

1314
export enum LoginErrors {
1415
AUTH_ERROR = "authError",
@@ -94,6 +95,7 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
9495
const oAuthEnabled = Boolean(
9596
authMethods?.github.enabled || authMethods?.oidc.enabled,
9697
)
98+
const passwordEnabled = authMethods?.password.enabled ?? true
9799

98100
// Hide password auth by default if any OAuth method is enabled
99101
const [showPasswordAuth, setShowPasswordAuth] = useState(!oAuthEnabled)
@@ -108,15 +110,15 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
108110
{loginPageTranslation.t("signInTo")}{" "}
109111
<strong>{commonTranslation.t("coder")}</strong>
110112
</h1>
111-
<Maybe condition={showPasswordAuth}>
113+
<Maybe condition={passwordEnabled && showPasswordAuth}>
112114
<PasswordSignInForm
113115
loginErrors={loginErrors}
114116
onSubmit={onSubmit}
115117
initialTouched={initialTouched}
116118
isLoading={isLoading}
117119
/>
118120
</Maybe>
119-
<Maybe condition={showPasswordAuth && oAuthEnabled}>
121+
<Maybe condition={passwordEnabled && showPasswordAuth && oAuthEnabled}>
120122
<div className={styles.divider}>
121123
<div className={styles.dividerLine} />
122124
<div className={styles.dividerLabel}>Or</div>
@@ -131,7 +133,14 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
131133
/>
132134
</Maybe>
133135

134-
<Maybe condition={!showPasswordAuth}>
136+
<Maybe condition={!passwordEnabled && !oAuthEnabled}>
137+
<AlertBanner
138+
severity="error"
139+
text="No authentication methods configured!"
140+
/>
141+
</Maybe>
142+
143+
<Maybe condition={passwordEnabled && !showPasswordAuth}>
135144
<div className={styles.divider}>
136145
<div className={styles.dividerLine} />
137146
<div className={styles.dividerLabel}>Or</div>

‎site/src/components/UsersLayout/UsersLayout.tsx

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import Link from "@material-ui/core/Link"
33
import { makeStyles } from "@material-ui/core/styles"
44
import GroupAdd from "@material-ui/icons/GroupAddOutlined"
55
import PersonAdd from "@material-ui/icons/PersonAddOutlined"
6+
import { useMachine } from "@xstate/react"
67
import { USERS_LINK } from "components/NavbarView/NavbarView"
78
import { PageHeader, PageHeaderTitle } from "components/PageHeader/PageHeader"
89
import { useFeatureVisibility } from "hooks/useFeatureVisibility"
@@ -15,13 +16,15 @@ import {
1516
useNavigate,
1617
} from "react-router-dom"
1718
import { combineClasses } from "util/combineClasses"
19+
import { authMethodsXService } from "xServices/auth/authMethodsXService"
1820
import { Margins } from "../../components/Margins/Margins"
1921
import { Stack } from "../../components/Stack/Stack"
2022

2123
export const UsersLayout: FC = () => {
2224
const styles = useStyles()
2325
const { createUser: canCreateUser, createGroup: canCreateGroup } =
2426
usePermissions()
27+
const [authMethods] = useMachine(authMethodsXService)
2528
const navigate = useNavigate()
2629
const { template_rbac: isTemplateRBACEnabled } = useFeatureVisibility()
2730

@@ -31,16 +34,17 @@ export const UsersLayout: FC = () => {
3134
<PageHeader
3235
actions={
3336
<>
34-
{canCreateUser && (
35-
<Button
36-
onClick={() => {
37-
navigate("/users/create")
38-
}}
39-
startIcon={<PersonAdd />}
40-
>
41-
Create user
42-
</Button>
43-
)}
37+
{canCreateUser &&
38+
authMethods.context.authMethods?.password.enabled && (
39+
<Button
40+
onClick={() => {
41+
navigate("/users/create")
42+
}}
43+
startIcon={<PersonAdd />}
44+
>
45+
Create user
46+
</Button>
47+
)}
4448
{canCreateGroup && isTemplateRBACEnabled && (
4549
<Link
4650
underline="none"

‎site/src/xServices/auth/authMethodsXService.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { assign, createMachine } from "xstate"
2+
import * as TypeGen from "api/typesGenerated"
3+
import * as API from "api/api"
4+
5+
export interface AuthMethodsContext {
6+
authMethods?: TypeGen.AuthMethods
7+
error?: Error | unknown
8+
}
9+
10+
export const authMethodsXService = createMachine(
11+
{
12+
id: "authMethods",
13+
predictableActionArguments: true,
14+
tsTypes: {} as import("./authMethodsXService.typegen").Typegen0,
15+
schema: {
16+
context: {} as AuthMethodsContext,
17+
services: {} as {
18+
getAuthMethods: {
19+
data: TypeGen.AuthMethods
20+
}
21+
},
22+
},
23+
context: {},
24+
initial: "gettingAuthMethods",
25+
states: {
26+
gettingAuthMethods: {
27+
invoke: {
28+
src: "getAuthMethods",
29+
onDone: {
30+
target: "idle",
31+
actions: ["assignAuthMethods"],
32+
},
33+
onError: {
34+
target: "idle",
35+
actions: ["setError"],
36+
},
37+
},
38+
},
39+
idle: {},
40+
},
41+
},
42+
{
43+
actions: {
44+
assignAuthMethods: assign({
45+
authMethods: (_, event) => event.data,
46+
}),
47+
setError: assign({
48+
error: (_, event) => event.data,
49+
}),
50+
},
51+
services: {
52+
getAuthMethods: () => API.getAuthMethods(),
53+
},
54+
},
55+
)

0 commit comments

Comments
 (0)