Skip to content
Open
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
5 changes: 3 additions & 2 deletions pkg/op/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package op
import (
"context"
"net/http"
"slices"

jose "github.com/go-jose/go-jose/v4"

Expand Down Expand Up @@ -105,7 +106,7 @@ func Scopes(c Configuration) []string {
if ok && provider.config.SupportedScopes != nil {
return provider.config.SupportedScopes
}
return DefaultSupportedScopes
return slices.Clone(DefaultSupportedScopes)
}

func ResponseTypes(c Configuration) []string {
Expand Down Expand Up @@ -230,7 +231,7 @@ func SupportedClaims(c Configuration) []string {
return provider.config.SupportedClaims
}

return DefaultSupportedClaims
return slices.Clone(DefaultSupportedClaims)
}

func CodeChallengeMethods(c Configuration) []oidc.CodeChallengeMethod {
Expand Down
8 changes: 5 additions & 3 deletions pkg/op/op.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,17 @@ func NewProvider(
NewAESCrypto(config.CryptoKey),
},
)
// Copied per provider: the options below assign into these and CORSOptions() hands its pointer to callers, so sharing the globals would let one provider reconfigure every other.
corsOpts := defaultCORSOptions
Comment on lines +272 to +273

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would like to see a copy/clone function for that so that this behavior is isolated.

o := &Provider{
config: config,
storage: storage,
accessTokenKeySet: keySet,
idTokenHinKeySet: keySet,
crypto: crypto,
endpoints: DefaultEndpoints,
endpoints: *DefaultEndpoints,
timer: make(<-chan time.Time),
corsOpts: &defaultCORSOptions,
corsOpts: &corsOpts,
}

for _, optFunc := range opOpts {
Expand All @@ -302,7 +304,7 @@ type Provider struct {
config *Config
issuer IssuerFromRequest
insecure bool
endpoints *Endpoints
endpoints Endpoints

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This changes the public contract and thus is a breaking change. We need to either keep it a pointer but copy the values, or merge this into next instead of main.

storage Storage
accessTokenKeySet oidc.KeySet
idTokenHinKeySet oidc.KeySet
Expand Down
70 changes: 70 additions & 0 deletions pkg/op/op_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net/http/httptest"
"net/url"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -469,3 +470,72 @@ func TestWithCustomEndpoints(t *testing.T) {
})
}
}

// A customisation must not reach the global or a sibling provider that never asked for it.
func TestNewProviderDoesNotMutateDefaultEndpoints(t *testing.T) {
defaultsBefore := *op.DefaultEndpoints

customAuth := op.NewEndpoint("/custom/authorize")
_, err := op.NewOpenIDProvider(testIssuer, testConfig,
storage.NewStorage(storage.NewUserStore(testIssuer)),
op.WithCustomAuthEndpoint(customAuth),
)
require.NoError(t, err)

assert.Equal(t, defaultsBefore, *op.DefaultEndpoints,
"DefaultEndpoints was mutated by WithCustomAuthEndpoint")

sibling, err := op.NewOpenIDProvider(testIssuer, testConfig,
storage.NewStorage(storage.NewUserStore(testIssuer)),
)
require.NoError(t, err)
assert.NotEqual(t, customAuth, sibling.AuthorizationEndpoint(),
"sibling provider inherited a customisation it did not request")
}

// WithCustom*Endpoint writes inside NewProvider raced with reads on an already-serving provider.
func TestNewProviderEndpointsConcurrentRace(t *testing.T) {
reader, err := op.NewOpenIDProvider(testIssuer, testConfig,
storage.NewStorage(storage.NewUserStore(testIssuer)),
)
require.NoError(t, err)

stop := make(chan struct{})
readerDone := make(chan struct{})
go func() {
defer close(readerDone)
for {
select {
case <-stop:
return
default:
_ = reader.AuthorizationEndpoint().Relative()
}
}
}()

const builders = 8
var builderWg sync.WaitGroup
builderWg.Add(builders)
for range builders {
Comment thread
wim07101993 marked this conversation as resolved.
go func() {
defer builderWg.Done()
_, err := op.NewOpenIDProvider(testIssuer, testConfig,
storage.NewStorage(storage.NewUserStore(testIssuer)),
op.WithCustomEndpoints(
op.NewEndpoint("/authorize"),
op.NewEndpoint("/oauth/token"),
op.NewEndpoint("/userinfo"),
op.NewEndpoint("/revoke"),
op.NewEndpoint("/end_session"),
op.NewEndpoint("/keys"),
),
)
assert.NoError(t, err)
}()
}

builderWg.Wait()
close(stop)
<-readerDone
}
3 changes: 2 additions & 1 deletion pkg/op/server_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@ func RegisterServer(server Server, endpoints Endpoints, options ...ServerOption)
decoder := schema.NewDecoder()
decoder.IgnoreUnknownKeys(true)

corsOpts := defaultCORSOptions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would like to see a copy/clone function for that so that this behavior is isolated.

ws := &webServer{
router: chi.NewRouter(),
server: server,
endpoints: endpoints,
decoder: decoder,
corsOpts: &defaultCORSOptions,
corsOpts: &corsOpts,
}

for _, option := range options {
Expand Down
Loading