Skip to content

Commit dd5b0b2

Browse files
coadlercoadler
authored
fix(scim): ensure scim users aren't created with their own org (coder#7595)
1 parent 0b15b1b commit dd5b0b2

File tree

8 files changed

+104
-25
lines changed

8 files changed

+104
-25
lines changed

‎coderd/apidoc/docs.go

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/userauth.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,11 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
921921
Username: params.Username,
922922
OrganizationID: organizationID,
923923
},
924-
LoginType: params.LoginType,
924+
// All of the userauth tests depend on this being able to create
925+
// the first organization. It shouldn't be possible in normal
926+
// operation.
927+
CreateOrganization: len(organizations) == 0,
928+
LoginType: params.LoginType,
925929
})
926930
if err != nil {
927931
return xerrors.Errorf("create user: %w", err)

‎coderd/users.go

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) {
128128
// Create an org for the first user.
129129
OrganizationID: uuid.Nil,
130130
},
131-
LoginType: database.LoginTypePassword,
131+
CreateOrganization: true,
132+
LoginType: database.LoginTypePassword,
132133
})
133134
if err != nil {
134135
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
@@ -313,19 +314,41 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) {
313314
return
314315
}
315316

316-
_, err = api.Database.GetOrganizationByID(ctx, req.OrganizationID)
317-
if httpapi.Is404Error(err) {
318-
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
319-
Message: fmt.Sprintf("Organization does not exist with the provided id %q.", req.OrganizationID),
320-
})
321-
return
322-
}
323-
if err != nil {
324-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
325-
Message: "Internal error fetching organization.",
326-
Detail: err.Error(),
327-
})
328-
return
317+
if req.OrganizationID != uuid.Nil {
318+
// If an organization was provided, make sure it exists.
319+
_, err := api.Database.GetOrganizationByID(ctx, req.OrganizationID)
320+
if err != nil {
321+
if httpapi.Is404Error(err) {
322+
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
323+
Message: fmt.Sprintf("Organization does not exist with the provided id %q.", req.OrganizationID),
324+
})
325+
return
326+
}
327+
328+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
329+
Message: "Internal error fetching organization.",
330+
Detail: err.Error(),
331+
})
332+
return
333+
}
334+
} else {
335+
// If no organization is provided, add the user to the first
336+
// organization.
337+
organizations, err := api.Database.GetOrganizations(ctx)
338+
if err != nil {
339+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
340+
Message: "Internal error fetching orgs.",
341+
Detail: err.Error(),
342+
})
343+
return
344+
}
345+
346+
if len(organizations) > 0 {
347+
// Add the user to the first organization. Once multi-organization
348+
// support is added, we should enable a configuration map of user
349+
// email to organization.
350+
req.OrganizationID = organizations[0].ID
351+
}
329352
}
330353

331354
err = userpassword.Validate(req.Password)
@@ -955,7 +978,8 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques
955978

956979
type CreateUserRequest struct {
957980
codersdk.CreateUserRequest
958-
LoginType database.LoginType
981+
CreateOrganization bool
982+
LoginType database.LoginType
959983
}
960984

961985
func (api *API) CreateUser(ctx context.Context, store database.Store, req CreateUserRequest) (database.User, uuid.UUID, error) {
@@ -964,6 +988,10 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create
964988
orgRoles := make([]string, 0)
965989
// If no organization is provided, create a new one for the user.
966990
if req.OrganizationID == uuid.Nil {
991+
if !req.CreateOrganization {
992+
return xerrors.Errorf("organization ID must be provided")
993+
}
994+
967995
organization, err := tx.InsertOrganization(ctx, database.InsertOrganizationParams{
968996
ID: uuid.New(),
969997
Name: req.Username,

‎coderd/users_test.go

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"time"
1111

1212
"github.com/google/uuid"
13+
"github.com/stretchr/testify/assert"
1314
"github.com/stretchr/testify/require"
1415
"golang.org/x/sync/errgroup"
1516

@@ -478,21 +479,49 @@ func TestPostUsers(t *testing.T) {
478479
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
479480
})
480481

482+
t.Run("CreateWithoutOrg", func(t *testing.T) {
483+
t.Parallel()
484+
auditor := audit.NewMock()
485+
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
486+
numLogs := len(auditor.AuditLogs())
487+
488+
firstUser := coderdtest.CreateFirstUser(t, client)
489+
numLogs++ // add an audit log for user create
490+
numLogs++ // add an audit log for login
491+
492+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
493+
defer cancel()
494+
495+
user, err := client.CreateUser(ctx, codersdk.CreateUserRequest{
496+
Email: "another@user.org",
497+
Username: "someone-else",
498+
Password: "SomeSecurePassword!",
499+
})
500+
require.NoError(t, err)
501+
502+
require.Len(t, auditor.AuditLogs(), numLogs)
503+
require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action)
504+
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-2].Action)
505+
506+
require.Len(t, user.OrganizationIDs, 1)
507+
assert.Equal(t, firstUser.OrganizationID, user.OrganizationIDs[0])
508+
})
509+
481510
t.Run("Create", func(t *testing.T) {
482511
t.Parallel()
483512
auditor := audit.NewMock()
484513
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
485514
numLogs := len(auditor.AuditLogs())
486515

487-
user := coderdtest.CreateFirstUser(t, client)
516+
firstUser := coderdtest.CreateFirstUser(t, client)
488517
numLogs++ // add an audit log for user create
489518
numLogs++ // add an audit log for login
490519

491520
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
492521
defer cancel()
493522

494-
_, err := client.CreateUser(ctx, codersdk.CreateUserRequest{
495-
OrganizationID: user.OrganizationID,
523+
user, err := client.CreateUser(ctx, codersdk.CreateUserRequest{
524+
OrganizationID: firstUser.OrganizationID,
496525
Email: "another@user.org",
497526
Username: "someone-else",
498527
Password: "SomeSecurePassword!",
@@ -502,6 +531,9 @@ func TestPostUsers(t *testing.T) {
502531
require.Len(t, auditor.AuditLogs(), numLogs)
503532
require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action)
504533
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-2].Action)
534+
535+
require.Len(t, user.OrganizationIDs, 1)
536+
assert.Equal(t, firstUser.OrganizationID, user.OrganizationIDs[0])
505537
})
506538

507539
t.Run("LastSeenAt", func(t *testing.T) {

‎codersdk/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ type CreateUserRequest struct {
6969
Email string `json:"email" validate:"required,email" format:"email"`
7070
Username string `json:"username" validate:"required,username"`
7171
Password string `json:"password" validate:"required"`
72-
OrganizationID uuid.UUID `json:"organization_id" validate:"required" format:"uuid"`
72+
OrganizationID uuid.UUID `json:"organization_id" validate:"" format:"uuid"`
7373
}
7474

7575
type UpdateUserProfileRequest struct {

‎docs/api/schemas.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1564,7 +1564,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
15641564
| Name | Type | Required | Restrictions | Description |
15651565
| ----------------- | ------ | -------- | ------------ | ----------- |
15661566
| `email` | string | true | | |
1567-
| `organization_id` | string | true | | |
1567+
| `organization_id` | string | false | | |
15681568
| `password` | string | true | | |
15691569
| `username` | string | true | | |
15701570

‎enterprise/coderd/scim.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,11 +156,27 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) {
156156
return
157157
}
158158

159+
var organizationID uuid.UUID
160+
//nolint:gocritic
161+
organizations, err := api.Database.GetOrganizations(dbauthz.AsSystemRestricted(ctx))
162+
if err != nil {
163+
_ = handlerutil.WriteError(rw, err)
164+
return
165+
}
166+
167+
if len(organizations) > 0 {
168+
// Add the user to the first organization. Once multi-organization
169+
// support is added, we should enable a configuration map of user
170+
// email to organization.
171+
organizationID = organizations[0].ID
172+
}
173+
159174
//nolint:gocritic // needed for SCIM
160175
user, _, err := api.AGPL.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, agpl.CreateUserRequest{
161176
CreateUserRequest: codersdk.CreateUserRequest{
162-
Username: sUser.UserName,
163-
Email: email,
177+
Username: sUser.UserName,
178+
Email: email,
179+
OrganizationID: organizationID,
164180
},
165181
LoginType: database.LoginTypeOIDC,
166182
})

0 commit comments

Comments
 (0)