feat: add Dex IdP module

[prskr]

What does this PR do?

Add a Dex module, similar to the already existing testcontainers-java module.

Why is it important?

This allows to test OAuth2/OpenID Connect authentication implementations.

Related issues

No issue was created for this so far.

How to test this PR

Hopefully the implemented tests and documentation is sufficient, if not, please let me know, so I can extend them 😊

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	ddcfd93
🔍 Latest deploy log	https://app.netlify.com/projects/testcontainers-go/deploys/6857eefeb0aeee00099c6e35
😎 Deploy Preview	https://deploy-preview-3216--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[stevenh]

Quick pass, just one the one ask for now.

[stevenh]

thanks for the quick changes, a bit more of an in depth pass this time.

[stevenh]

suggestion: keep this private, no need to expand the API surface area by exporting.

[prskr]

Making this error private would mean, you cannot check against it and in consequence it'd make no sense to declare it as a variable in the first place?
I'd prefer to keep it that way to allow users to check for errors.Is(err, dex.ErrPasswordRequired)

[stevenh]

Yes, but the question is why would a user need to check the error, it failed because the code didn't pass the correct values in, so will need to be fixed to address the issue.

What scenario are you envisaging that checking the error would help with?

[prskr]

Alright, will make it private

[stevenh]

suggestion: use parameters instead of structures as that makes it clear what the user needs to pass, using a struct the caller can easily pass an empty struct with likely unexpected results.

[prskr]

I disagree, when using parameters instead of a struct, this would mean:

• either clientID and clientSecret must be set or it is allowed to pass empty strings and the function is checking for it and generating them (as it is now) but IMHO that does not improve the readability of the code in any way (empty strings vs. empty struct?!)
• every time Dex adds new fields to the client app this would potentially mean a breaking change of the public API of this module (at least for mandatory fields) or it would be necessary to add a variadic options API for this function as well to pass optional parameters, making it more confusing to consume

on the other hand (see also CreatePassword ):

• if mandatory fields are missing, it's easy to return an error
• if more fields are becoming mandatory, it would also be possible to add a Validate() function to the request structs to allow users of the module to check up front if their request is valid
• generally, it is always possible to add new fields without a breaking change in the API

Also, too many parameters in a public API are making it hard to use, for the CreateClientApp we'd be talking about 6 parameters, with the context.Context actually being the only one that is mandatory ( I double checked, actually even the generation of the Id in my function is not even necessary, as Dex is generating it if necessary) so how would you communicate that to the caller?

[stevenh]

Sounds like the same direction which was take with the container request type, which is something we're in the process of changing, because it became unmanageable and very hard for the user to understand.

Could you clarify how each of the fields is used, CreateClientAppRequest and CreatePasswordRequest, specifically which are optional, and which may have and exclusive requirement. For example it sounds like Password should never be set if Hash is.

If you've not read it this article describes the benefits of functional options, which is what I'm thinking would be the better option here and is the direction we're going with all testcontainers core.

[prskr]

I am well aware of the pattern, thank you.

As for the usage:

• for the CreatePasswort request email and password or hash are required for a user to login
• for the CreateClientApp, all fields are optional and will be generated on demand if necessary ( ClientID and ClientSecret)

Honestly, I'm this I am not willing to discuss, if you insist on the options pattern, I will just remove the functions altogether or probably close the PR

[mdelapenya]

@prskr we are transitioning to functional opts when/if possible, so I'd suggest using them here. If you see other modules in the repo, we usually define an options struct receiving the optional values, including custom Opts setting them. I think this pattern, which could be seen as "boring", makes the API pretty predictable.

You are always able to create your own module under your account, with your own APIs, and we can link to it in the modules catalog as a community module, but having it here will mean automatic releases, CI execution, and more visibility, among others.

In any case, we'd love to see your contribution in the repo, if possible.

[prskr]

I totally get what you're doing for the "containers" API and I don't want to question that or whatever. My only problem with the approach - especially in this context - is that if you have multiple option types in a single package all starting with 'WithXXX' in my opinion does not improve the developer experience in any way because (AFAIK) gopls for instance does not filter for functions/variables matching the option type but it filters solely for functions/variables matching 'With' so it's up to the developer to check which options can be used with the current function. Apart from that I continuously find new options in APIs that I'm using probably because they are in other packages or because they don't follow the naming scheme or because I was confronted with too many god damn options so I couldn't easily browse them all.

The point where you manually create documentation for all possible options because Godoc isn't helping should alert you that something's off.

And this problem is getting even worse when you have multiple option types because some option might sound correctly but the type does not match (or not obviously match because some "smart" type hacks were applied).

I'm not saying you should not use this for the container API, I even created some options for this module, I just don't see the point of claiming there's this single hammer and everything is a nail. CreatePassword has 2 mandatory parameters and...4 optional ones, CreateClientApp only has optional parameters... I'm currently on vacation and don't have a laptop with me but I'm happy to create an example of how the API would Look Like to illustrate how inconvenient that could become... no

Edit: CreateCollection is actually a great example, is this really easier to understand and review than passing a struct with named fields that would tell you what this 'true' means or that 'map[string]any'? No? I thought so too...probably I should close this PR and implement the one integration test I need with a generic container...

[stevenh]

I'm not sure I follow, when I look at the docs all the options are grouped under the type:

Also grouped together in the index, so if you're looking for say an CreatePasswordAuth option, the docs make it clear the options you have:

I'm interested to hear more about the issue you're seeing with options spread across packages, have you got any examples of that?

I just tried in VS code using the options in the example and prompted with the relevant options, so again interested to hear about that issue more, so we can understand what you're experiencing.

Definitely interested in what it might look like, when you get back, in the mean time enjoy your holiday and home you have some 🌞

[stevenh]

suggestion: as above parameters instead of struct, also solves the password / hash as you can have a parameter which is either, but not both.

[prskr]

see also previous comment.

Also for the password or hash question, it does not really make sense to me?
The Hash needs to be a proper Bcrypt hash, whereas the Password can be some arbitrary string.

If unifying it to a single parameter I'd have to rely on some heuristics to determine whether the string is a Bcrypt Hash or if it is meant to be the Password for no good reason because it's perfectly possible to pass either a Password or a Hash in their corresponding fields.

Also, passing the Password as string and the Hash as []byte clarifies and distinguishes their individual characteristics to the caller. Apart from being able to clear the Hash after usage as a nice best practice even though this is not really security critical in this context.

So, I'd rather keep it as is for the mentioned reasons.

[stevenh]

question: could the default be set on c and overridden with a functional option if needed?

[prskr]

theoretically yes but I don't think it would improve anything.

I won't ague again about option pattern APIs 😅 but I also don't see a realistic use case.

My bet would be that 99.9% of the users won't ever touch or even think about using a customhttp.Client here and if they have some use case for that (like...transparently retrying the discovery document retrieval call in their test suite 😅 ) I would even argue that having to pass different http.Client instances to multiple calls is rather a code flaw than a 'feature'.

And even if they would need something like this, they could simply copy the dex.Container struct and use different http.Client instances on the struct level rather than on function level...

So, if you don't have a hard reason for that or insist on changing that, I'd prefer to keep it as is

[stevenh]

sounds like you're suggesting the customisation of client isn't needed, as it doesn't have a well defined use case. If thats the case I'd remove. We can add in later if a case requires it.

I've see so many cases where a developer, myself included, thinks lets add this option because its not hard to do, only for that option to never be used. So with all of the years under my belt now I always ask, but is that actually needed.

[prskr]

Sure, with all the experience, who am I to question that, will remove it

[mdelapenya]

@prskr our job as maintainers is to keep consistency across the codebase, because we want to make sure contributors create a consistent experience while using the project.

I'm sorry if we are adding too many questions here or in any other PR, so please forgive us if we are many times too verbose. Also, we follow conventional comments, which if not used to, could be seen as very "direct", but I do confirm that once familiar with the style it's pretty simple to understand that we are talking just about code, not about anything else.

Said that, we are questionable, so we are really glad to see you interact with us explaining your concerns. The resulting code will be richer and with more context.

[prskr]

I get that, but all I saw is "suggestion: " which implies I can do that or not, combined with "requested changes" so it's not a suggestion, it is "change this or it won't be merged" which is also fine but then make it consistent please 😊

Also, sorry to be frank but, it's quite common to make it possible to switch the http.Client for various reasons, I don't want to start explaining every common design decision of the Go Community over and over again...

Edit: I'm German, I'm all in for direct comments, I'm just getting annoyed if the comments aren't leading anywhere but just extend the discussion forever for no good reason

[mdelapenya]

@prskr I'm Spanish and I'm more friendly in my communication style. And I do appreciate the same no matter the nationality.

[stevenh]

I believe it's been mentioned before but the prefixes are defined by the conventional comments standard, which we follow for this project. I would definitely encourage you to familiarise with that, if you aren't already.

We never know what contributors have considered when developing a potential change, so one of our roles is to ask the questions, make suggestions with goal of making testcontainers-go better for all. This can involve helping to ensure we provide a consistent style and approach across the codebase, so users can apply knowledge from using one module, or the core, to another resulting in a better developer experience.

We always do so with best intent, so I would encourage you to engage with the team in a respectful and courteous manor. Opinions may differ, we may not start out aligned, we may have missed a key fact when comments are made, if you believe this is the case I would encourage you to present the details to build that shared understanding. A well formed case for a different approach will always be considered.

My explanation of how functional options could be applied to the user creation, is hopefully a good example of that, in that I presented what the code might look like and explained how it can help users avoid runtime issues. You suggested that these might have a discoverability issue, and I'm interested to understand more about that, as I've not experienced that personally. That doesn't mean I'm right and you're wrong, just that we need to share our collective experiences so the other person can understand the alternative perspective.

We might have strong opinions but they are loosely held, and we want to understand and learn from others.

Most importantly we're all here to make testcontainers-go better, every contribution is valuable, it's at the heart of open source so thank you for you work so far on this and I hope we can work together to get it over the line.

[prskr]

Okay: it is quite common to create some kind of "escape hatch" to explicitly use a provided http.Client instead if the global one for multiple reasons.
Due to the nature of the default http.Client being a global one and the arguably good or bad practice (I don't want to argue about that) that certain libraries are patching the global http.Client with certain behaviors, many developers prefer to specify the http.Client (for instance with an "empty" default). This ensures that the client behaves as expected and does not for instance automatically retry requests, tries to submit telemetry or emit log lines, that could make test debugging a lot easier. Hence, I figured, that would be some valuable option. Of course, if you think different, I'm happy to remove this.

[stevenh]

I wanted to take the time to provide an example of what I had in mind, to make sure we're aligned on what I suggested and how it could help users.

As @mdelapenya mentioned this is a approach we're looking to standardise on to provide a consistent user experience across all testcontainer modules.

While this is more initial code, it makes it obvious at a glance what the user needs to use the method, specifically:

1. Email is required
2. Either a password or a hash are required
3. UserID and Username are optional

Here's it in use:

if err := dexContainer.CreateLogin(ctx, email, dex.WithPassword(password), dex.WithUsername("ted.tester")); err != nil {
	log.Printf("failed to create login: %s", err)
	return
}

This approach helps prevent more issues at compile time instead of runtime, for example it enforces an auth option is provided. Also if the underlying API changes, adding or removing options we are more likely, can update in a way which maintains compatibility.

Hope this helps.

Suggested change

	// CreatePassword registers a new 'password' (login) in Dex. Either Password or Hash must be provided.
	// If Hash is provided, Password will be ignored. If UserID is not set, a UUID will be generated.
	// The password will be hashed using bcrypt if provided. Returns ErrPasswordRequired if neither
	// Password nor Hash are provided, or an error from bcrypt if hashing fails.
	func (c Container) CreatePassword(
	ctx context.Context,
	req CreatePasswordRequest,
	) (err error) {
	apiClient, connCloser, err := c.createDexAPIClient(ctx)
	if err != nil {
	return fmt.Errorf("create Dex API client: %w", err)
	}
	defer func() {
	if closeErr := connCloser.Close(); closeErr != nil {
	err = errors.Join(err, fmt.Errorf("close GRPC connection: %w", closeErr))
	}
	}()
	if req.Password == "" && len(req.Hash) == 0 {
	return ErrPasswordRequired
	}
	if req.UserID == "" {
	req.UserID = uuid.New().String()
	}
	if len(req.Hash) == 0 {
	req.Hash, err = bcrypt.GenerateFromPassword([]byte(req.Password), bcryptCost)
	if err != nil {
	return fmt.Errorf("hash plaintext password with bcrypt: %w", err)
	}
	}
	apiReq := &dexapi.CreatePasswordReq{
	Password: &dexapi.Password{
	UserId: req.UserID,
	Username: req.Username,
	Email: req.Email,
	Hash: req.Hash,
	},
	}
	if _, err = apiClient.CreatePassword(ctx, apiReq); err != nil {
	return fmt.Errorf("create password in Dex: %w", err)
	}
	clear(req.Hash)
	return nil
	}
	// createPasswordRequest represents a request to register an identity in Dex.
	type createPasswordRequest struct {
	userID string
	email string
	username string
	// Password is a plain text password.
	password string
	// Hash is a bcrypt hash of the password.
	hash []byte
	}
	// CreatePasswordOption represents a create password optional parameter.
	type CreatePasswordOption func(*createPasswordRequest) error
	// CreatePasswordAuth represents a create password authentication parameter.
	type CreatePasswordAuth func(*createPasswordRequest) error
	// WithPassword sets the plaintext password for the password request.
	func WithPassword(password string) CreatePasswordAuth {
	return func(req *createPasswordRequest) error {
	if password == "" {
	return errors.New("password cannot be empty")
	}
	req.password = password
	return nil
	}
	}
	// WithHash sets the bcrypt hash for the password request.
	func WithHash(hash []byte) CreatePasswordAuth {
	return func(req *createPasswordRequest) error {
	if len(hash) == 0 {
	return errors.New("hash cannot be empty")
	}
	req.hash = hash
	return nil
	}
	}
	// WithUserID sets the user ID for the password request.
	func WithUserID(userID string) CreatePasswordOption {
	return func(req *createPasswordRequest) error {
	if userID == "" {
	return fmt.Errorf("userID cannot be empty")
	}
	req.userID = userID
	return nil
	}
	}
	// WithUsername sets the username for the password request.
	func WithUsername(username string) CreatePasswordOption {
	return func(req *createPasswordRequest) error {
	if username == "" {
	return errors.New("username cannot be empty")
	}
	req.username = username
	return nil
	}
	}
	// CreateLogin registers a new login in Dex.
	// It requires either a plaintext password or a bcrypt hash of the password.
	// If the password is provided, it will be hashed using bcrypt before being sent to Dex.
	// If the hash is provided, it will be used directly.
	// The user ID is generated if not provided, and the username can be set optionally.
	func (c Container) CreateLogin(ctx context.Context, email string, auth CreatePasswordAuth, options ...CreatePasswordOption) (err error) {
	req := &createPasswordRequest{
	email: email,
	userID: uuid.New().String(),
	}
	for _, opt := range options {
	if err = opt(req); err != nil {
	return fmt.Errorf("apply option: %w", err)
	}
	}
	apiClient, connCloser, err := c.createDexAPIClient(ctx)
	if err != nil {
	return fmt.Errorf("create Dex API client: %w", err)
	}
	defer func() {
	if closeErr := connCloser.Close(); closeErr != nil {
	err = errors.Join(err, fmt.Errorf("close GRPC connection: %w", closeErr))
	}
	}()
	if len(req.hash) == 0 {
	req.hash, err = bcrypt.GenerateFromPassword([]byte(req.password), bcryptCost)
	if err != nil {
	return fmt.Errorf("hash plaintext password with bcrypt: %w", err)
	}
	}
	apiReq := &dexapi.CreatePasswordReq{
	Password: &dexapi.Password{
	UserId: req.userID,
	Username: req.username,
	Email: req.email,
	Hash: req.hash,
	},
	}
	if _, err = apiClient.CreatePassword(ctx, apiReq); err != nil {
	return fmt.Errorf("create password in Dex: %w", err)
	}
	return nil
	}

[prskr]

I am aware what you have in mind and how the options pattern is implemented (although I prefer interfaces over pure functions because that is slightly more flexible) but I still fail to see how that catches anything at compile time unless you replace the option for password or hash with an actual 'credential' type with two implementations (e.g. PlainTextCredential and HashCredential) but the options pattern does not solve this in any way 🤷‍♂️

Plus - but that is quite personal - I don't Like how that forces (in most cases) multi-line function calls that also don't improve error handling readability...

[stevenh]

Let me try to clarify, with the struct there is no compile time validation, if you didn't set either a password or hash on the struct you will only know when the code runs. The user has to read the comments to know they have to set one or the other.

With the options version, first it's self documenting, as there is auth parameter which must be provided, if you don't, or provide an invalid option it won't even compile. Yes you could still pass a blank string to the methods, but that would be a conscious action vs just passing in a default struct with no values set.

Below is the options version, as you can see it enforces you have to pass auth which has two valid options, WithPassword and WithHash, the other truly optional values are seperate.

func (c Container) CreateLogin(ctx context.Context, email string, auth CreatePasswordAuth, options ...CreatePasswordOption) (err error) {

[prskr]

I already commented I will do everything as requested. Just for the record as also already mentioned @mdelapenya there are no "suggestions" here, most of the time it does not even look like you'd even read what I explained, it's 100% pushing into "do this like I want, or fuck off" and as a consequence this will certainly be the last PR I will do for test containers. I already had a similar experience with testcontainers-dotnet where I discussed for at least 3 months with the maintainer about every tiny API specific (and back then I didn't even discuss much, I just directly changed everything as requested) and nevertheless the PR was actually never merged at all because I eventually moved on and solved my problem in a different way. If that is how you want to live open source, alright, just my 2 cents.

[mdelapenya]

@prskr I would like to be candid for a moment. Discussing the technical details for every tiny API specific is the right thing to do when contributing to a project that is so widely used. If not, we can get to an API that is super difficult to use/read/maintain, like the API for the Podman support you and me introduced a couple of years ago. We are paying the added complexity at the provider layer so hard, that I'm removing the entire provider layer the soonest. And at that time I was not picky for any tiny API specific, but encouraging you to contribute more. The result? You merged what you wanted and did not appear back until now. So it's not about being German, Spanish or British, it's about being polite with the OSS community here at Testcontainers Go. I can be paid by my employer to do this, and you can tell me that. But @stevenh is using is very reduced spare time to respond, read all the tiny API specifics of any PR he takes, and do so much amazing work that he does not deserve how you are responding to him.

As I said in #3216 (comment), if you're not really convinced, like it already happened here, and the .NET example you brought, you can always use your repo with your own API, like the wiremocks or microcks folks did, although for obvious different reasons.

I'll let you do what you consider it's fine, we are not gonna close this as we see the value in adding modules to the catalog.

I want to finish saying sorry for being rude, but I had to defend the work of the community here.

[prskr]

No need to apologize, I rather get honest critic/feedback than polite tip toeing!
I'm honestly sorry that the Podman support caused you so many troubles and I wasn't available to at least share that pain. For what it's worth: back then I switched job and my new employer does not use go or testcontainers and I barely had some time left to contribute to anything OSS in the last years. In fact, I spent the last 4 years convincing my superiors that a company that big should definitely contribute to open source...It wasn't for the lack of interest but simply time. So, sorry!

I get that the API needs to be good as it's hard/impossible to change once it's published and I'm as much concerned about the API as you, are otherwise I'd have blindly accepted what was proposed without pushing back. If the options pattern is your only way to go, alright, but please document it then also accordingly and don't claim it's a suggestion when it's a requirement really. With the little time I have to do open source stuff, I'd like to be as efficient as possible, that also means, the fewer iterations I have to incorporate review comments and such, the better, if that makes sense.

[prskr]

I'll change everything as you requested when I'm back from vacation, hopefully this will then at least guarantee a quick merge although I'll detest my own API then