LDAP: Support Account Attribute Maps

[hugoghx]

This PR enables CLI support for account attribute maps and also takes the LDAP server's user attributes into account when authenticating with an LDAP auth method

[louisruch]

One nit, otherwise this looks great. Please also add a changelog entry for this

[louisruch]

nit: have a switch here, possibly even include a default that returns an error

[hugoghx]

I don't think we should error if we get an unexpected attribute from the LDAP server - We should just ignore it, otherwise we could get into a situation where, for example, an LDAP setup for multiple downstream services could be incompatible with Boundary.

[jimlambrt]

I believe convertAccountAttributeMaps calls ConvertToAccountToAttribute
which will validate the attribs and error if there's an unexpected one.

There's also a column constraint to_attribute_valid_values that validates the
valid values in the db.

Given these two constraints, it seems reasonable for us to have a switch here that
explicity returns an error before sending an invalid transaction to the db.

[jimlambrt]

I believe convertAccountAttributeMaps calls ConvertToAccountToAttribute
which will validate the attribs and error if there's an unexpected one.

There's also a column constraint to_attribute_valid_values that validates the
valid values in the db.

Given these two constraints, it seems reasonable for us to have a switch here that
explicity returns an error before sending an invalid transaction to the db.

[hugoghx]

@jimlambrt Wouldn't that default case essentially be dead code? The pre-requisite for this new code to run is that err == nil from convertAccountAttributeMaps (https://github.com/hashicorp/boundary/pull/4788/files#diff-bcf83b5283886c98fc45b6435fdc94d87d545b2768c6beac8acd14eb9d6b13f1R115). If we error in convertAccountAttributeMaps, this code will never run - Maybe we could surface the error instead of ignoring it here?

[jimlambrt]

A few things come to mind. First, you have no tests which ensure that Authenticate returns an error before sending an invalid transaction to the db. Next, the current implementation of Authenticate is ambiguous in how it handles the invariant which is easily demonstrable given the discourse between you and Louis. Additionally, depth and defense is a well known and accepted practice when it comes to enforcing domain invariants. Finally, I'm not sure I totally agree with your use of dead code in this situation since it relies on data-flow analysis which can easily change unexpectedly in the future (see the previously referenced depth and defense pattern).

[hugoghx]

@jimlambrt

First, you have no tests which ensure that Authenticate returns an error before sending an invalid transaction to the db.

I wasn't able to write a test to this unfortunately. The account map test state required to produce an error is invalid at multiple levels (TestAuthMethod errors when you pass in invalid account attributes, and even if we bypassed that, we'd get a DB error as you mentioned the to_attribute_valid_values constraint in the DB which enforces that any to_attribute be either email or fullname), so I'm unsure how to produce a test with the required state for an error. Do you have any ideas on how we could produce the invalid state for the test?

Next, the current implementation of Authenticate is ambiguous in how it handles the invariant which is easily demonstrable given the discourse between you and Louis. Additionally, depth and defense is a well known and accepted practice when it comes to enforcing domain invariants. Finally, I'm not sure I totally agree with your use of dead code in this situation since it relies on data-flow analysis which can easily change unexpectedly in the future (see the previously referenced depth and defense pattern).

This is very fair - With depth and defense patterns in mind, should we handle and surface the error from convertAccountAttributeMaps and, in addition, ensure a switch default case that throws an error exists?

ie:

attrMaps, err := am.convertAccountAttributeMaps(ctx)
		if err != nil {
			return nil, errors.Wrap(ctx, err, op, errors.WithMsg("failed to convert account attribute maps"))
		}

		for _, attrMap := range attrMaps {
			aam := attrMap.(*AccountAttributeMap)
			switch aam.ToAttribute {
			case DefaultEmailAttribute:
				emailAttr = aam.FromAttribute
			case DefaultFullNameAttribute:
				fullNameAttr = aam.FromAttribute
			default:
				return nil, errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("invalid to attribute %q", aam.ToAttribute))
			}
		}

[hugoghx]

@jimlambrt See changes with 6d8dd73

[jimlambrt]

@jimlambrt See changes with 6d8dd73

Let me know when you're ready for re-review.

[hugoghx]

@jimlambrt Ready to re-review - I was able to write that test we talked about using sqlmock

[jimlambrt]

ty!