Skip to content

chore: implement organization sync and create idpsync package #14432

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 23 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (

"cdr.dev/slog"
"cdr.dev/slog/sloggers/sloghuman"
"github.com/coder/coder/v2/coderd/entitlements"
"github.com/coder/pretty"
"github.com/coder/quartz"
"github.com/coder/retry"
Expand Down Expand Up @@ -605,6 +606,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
SSHConfigOptions: configSSHOptions,
},
AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(),
Entitlements: entitlements.New(),
NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled.
}
if httpServers.TLSConfig != nil {
Expand Down
13 changes: 13 additions & 0 deletions cli/testdata/coder_server_--help.golden
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,11 @@ OIDC OPTIONS:
groups. This filter is applied after the group mapping and before the
regex filter.

--oidc-organization-assign-default bool, $CODER_OIDC_ORGANIZATION_ASSIGN_DEFAULT (default: true)
If set to true, users will always be added to the default
organization. If organization sync is enabled, then the default org is
always added to the user's set of expectedorganizations.

--oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"})
OIDC auth URL parameters to pass to the upstream provider.

Expand Down Expand Up @@ -479,6 +484,14 @@ OIDC OPTIONS:
--oidc-name-field string, $CODER_OIDC_NAME_FIELD (default: name)
OIDC claim field to use as the name.

--oidc-organization-field string, $CODER_OIDC_ORGANIZATION_FIELD
This field must be set if using the organization sync feature. Set to
the claim to be used for organizations.

--oidc-organization-mapping struct[map[string][]uuid.UUID], $CODER_OIDC_ORGANIZATION_MAPPING (default: {})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is it possible for this to be just map[string][]uuid.UUID?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need this instead of solely relying on the --oidc-organization-field? Orgs are uniquely named right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to use the type serpent.Struct[map[string][]uuid.UUID] to be compatible with all parsing.

Can you explain why we need this instead of solely relying on the --oidc-organization-field? Orgs are uniquely named right?

I could use names, but that assumes the IDP will use Coder organization names in their claims, which we've found with groups that this almost never the case.

A map of OIDC claims and the organizations in Coder it should map to.
This is required because organization IDs must be used within Coder.

--oidc-group-regex-filter regexp, $CODER_OIDC_GROUP_REGEX_FILTER (default: .*)
If provided any group name not matching the regex is ignored. This
allows for filtering out groups that are not needed. This filter is
Expand Down
13 changes: 13 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,19 @@ oidc:
# Ignore the userinfo endpoint and only use the ID token for user information.
# (default: false, type: bool)
ignoreUserInfo: false
# This field must be set if using the organization sync feature. Set to the claim
# to be used for organizations.
# (default: <unset>, type: string)
organizationField: ""
# If set to true, users will always be added to the default organization. If
# organization sync is enabled, then the default org is always added to the user's
# set of expectedorganizations.
# (default: true, type: bool)
organizationAssignDefault: true
# A map of OIDC claims and the organizations in Coder it should map to. This is
# required because organization IDs must be used within Coder.
# (default: {}, type: struct[map[string][]uuid.UUID])
organizationMapping: {}
# This field must be set if using the group sync feature and the scope name is not
# 'groups'. Set to the claim to be used for groups.
# (default: <unset>, type: string)
Expand Down
9 changes: 9 additions & 0 deletions coderd/apidoc/docs.go

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

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

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

11 changes: 11 additions & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (

"cdr.dev/slog"
"github.com/coder/coder/v2/coderd/entitlements"
"github.com/coder/coder/v2/coderd/idpsync"
"github.com/coder/quartz"
"github.com/coder/serpent"

Expand Down Expand Up @@ -243,6 +244,9 @@ type Options struct {
WorkspaceUsageTracker *workspacestats.UsageTracker
// NotificationsEnqueuer handles enqueueing notifications for delivery by SMTP, webhook, etc.
NotificationsEnqueuer notifications.Enqueuer

// IDPSync holds all configured values for syncing external IDP users into Coder.
IDPSync idpsync.IDPSync
}

// @title Coder API
Expand Down Expand Up @@ -270,6 +274,13 @@ func New(options *Options) *API {
if options.Entitlements == nil {
options.Entitlements = entitlements.New()
}
if options.IDPSync == nil {
options.IDPSync = idpsync.NewAGPLSync(options.Logger, idpsync.SyncSettings{
OrganizationField: options.DeploymentValues.OIDC.OrganizationField.Value(),
OrganizationMapping: options.DeploymentValues.OIDC.OrganizationMapping.Value,
OrganizationAssignDefault: options.DeploymentValues.OIDC.OrganizationAssignDefault.Value(),
})
}
if options.NewTicker == nil {
options.NewTicker = func(duration time.Duration) (tick <-chan time.Time, done func()) {
ticker := time.NewTicker(duration)
Expand Down
2 changes: 1 addition & 1 deletion coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ var (
rbac.ResourceAssignOrgRole.Type: rbac.ResourceAssignOrgRole.AvailableActions(),
rbac.ResourceSystem.Type: {policy.WildcardSymbol},
rbac.ResourceOrganization.Type: {policy.ActionCreate, policy.ActionRead},
rbac.ResourceOrganizationMember.Type: {policy.ActionCreate},
rbac.ResourceOrganizationMember.Type: {policy.ActionCreate, policy.ActionDelete, policy.ActionRead},
rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionUpdate},
rbac.ResourceProvisionerKeys.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionDelete},
rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(),
Expand Down
172 changes: 172 additions & 0 deletions coderd/idpsync/idpsync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
package idpsync

import (
"context"
"net/http"
"strings"

"github.com/golang-jwt/jwt/v4"
"github.com/google/uuid"
"golang.org/x/xerrors"

"cdr.dev/slog"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/httpapi"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/site"
)

// IDPSync is an interface, so we can implement this as AGPL and as enterprise,
// and just swap the underlying implementation.
// IDPSync exists to contain all the logic for mapping a user's external IDP
// claims to the internal representation of a user in Coder.
// TODO: Move group + role sync into this interface.
type IDPSync interface {
OrganizationSyncEnabled() bool
// ParseOrganizationClaims takes claims from an OIDC provider, and returns the
// organization sync params for assigning users into organizations.
ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError)
// SyncOrganizations assigns and removed users from organizations based on the
// provided params.
SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error
}

// AGPLIDPSync is the configuration for syncing user information from an external
// IDP. All related code to syncing user information should be in this package.
type AGPLIDPSync struct {
Logger slog.Logger

SyncSettings
}

type SyncSettings struct {
// OrganizationField selects the claim field to be used as the created user's
// organizations. If the field is the empty string, then no organization updates
// will ever come from the OIDC provider.
OrganizationField string
// OrganizationMapping controls how organizations returned by the OIDC provider get mapped
OrganizationMapping map[string][]uuid.UUID
// OrganizationAssignDefault will ensure all users that authenticate will be
// placed into the default organization. This is mostly a hack to support
// legacy deployments.
OrganizationAssignDefault bool
}

type OrganizationParams struct {
// SyncEnabled if false will skip syncing the user's organizations.
SyncEnabled bool
// IncludeDefault is primarily for single org deployments. It will ensure
// a user is always inserted into the default org.
IncludeDefault bool
// Organizations is the list of organizations the user should be a member of
// assuming syncing is turned on.
Organizations []uuid.UUID
}

func NewAGPLSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync {
return &AGPLIDPSync{
Logger: logger.Named("idp-sync"),
SyncSettings: settings,
}
}

// ParseStringSliceClaim parses the claim for groups and roles, expected []string.
//
// Some providers like ADFS return a single string instead of an array if there
// is only 1 element. So this function handles the edge cases.
func ParseStringSliceClaim(claim interface{}) ([]string, error) {
groups := make([]string, 0)
if claim == nil {
return groups, nil
}

// The simple case is the type is exactly what we expected
asStringArray, ok := claim.([]string)
if ok {
return asStringArray, nil
}

asArray, ok := claim.([]interface{})
if ok {
for i, item := range asArray {
asString, ok := item.(string)
if !ok {
return nil, xerrors.Errorf("invalid claim type. Element %d expected a string, got: %T", i, item)
}
groups = append(groups, asString)
}
return groups, nil
}

asString, ok := claim.(string)
if ok {
if asString == "" {
// Empty string should be 0 groups.
return []string{}, nil
}
// If it is a single string, first check if it is a csv.
// If a user hits this, it is likely a misconfiguration and they need
// to reconfigure their IDP to send an array instead.
if strings.Contains(asString, ",") {
return nil, xerrors.Errorf("invalid claim type. Got a csv string (%q), change this claim to return an array of strings instead.", asString)
}
return []string{asString}, nil
}

// Not sure what the user gave us.
return nil, xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T", claim)
}

// IsHTTPError handles us being inconsistent with returning errors as values or
// pointers.
func IsHTTPError(err error) *HTTPError {
var httpErr HTTPError
if xerrors.As(err, &httpErr) {
return &httpErr
}

var httpErrPtr *HTTPError
if xerrors.As(err, &httpErrPtr) {
return httpErrPtr
}
return nil
}

// HTTPError is a helper struct for returning errors from the IDP sync process.
// A regular error is not sufficient because many of these errors are surfaced
// to a user logging in, and the errors should be descriptive.
type HTTPError struct {
Code int
Msg string
Detail string
RenderStaticPage bool
RenderDetailMarkdown bool
}

func (e HTTPError) Write(rw http.ResponseWriter, r *http.Request) {
if e.RenderStaticPage {
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
Status: e.Code,
HideStatus: true,
Title: e.Msg,
Description: e.Detail,
RetryEnabled: false,
DashboardURL: "/login",

RenderDescriptionMarkdown: e.RenderDetailMarkdown,
})
return
}
httpapi.Write(r.Context(), rw, e.Code, codersdk.Response{
Message: e.Msg,
Detail: e.Detail,
})
}

func (e HTTPError) Error() string {
if e.Detail != "" {
return e.Detail
}

return e.Msg
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package coderd
package idpsync_test

import (
"encoding/json"
"testing"

"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/coderd/idpsync"
)

func TestParseStringSliceClaim(t *testing.T) {
Expand Down Expand Up @@ -123,7 +125,7 @@ func TestParseStringSliceClaim(t *testing.T) {
require.NoError(t, err, "unmarshal json claim")
}

found, err := parseStringSliceClaim(c.GoClaim)
found, err := idpsync.ParseStringSliceClaim(c.GoClaim)
if c.ErrorExpected {
require.Error(t, err)
} else {
Expand All @@ -133,3 +135,13 @@ func TestParseStringSliceClaim(t *testing.T) {
})
}
}

func TestIsHTTPError(t *testing.T) {
t.Parallel()

herr := idpsync.HTTPError{}
require.NotNil(t, idpsync.IsHTTPError(herr))
require.NotNil(t, idpsync.IsHTTPError(&herr))

require.Nil(t, error(nil))
}
Loading
Loading