Add Custom Identity Provider Mapper

[bl00mber]

close #382, close #434

[hamiltont]

@bl00mber be aware, quite some time ago I put a bounty on one of the isues you're working to close. If this PR eventually gets merged you can claim it. Not enough to pay for the coding, more of a "have some beer/coff/etc, thanks for fixing something that was a problem for me"

[bl00mber]

@hamiltont yeah, thanks.

[bl00mber]

@mrparkers

[mrparkers]

Hey @bl00mber, thanks for the PR. Apologies for the delayed review.

I'm struggling a bit with understanding how well this change solves the problem described in #382. My understanding of the problem here is that we'd like this provider to support custom identity provider mappers (similar to how the keycloak_custom_user_federation resource supports custom implemented user federation providers). But extending the existing keycloak_*_identity_provider_mapper resources with this new attribute seems more like a bandaid solution at best - a custom identity provider mapper would still have to pick one of these resources, fill in the "required" attributes with dummy data, and put their real config within the extra_config attribute.

A better solution seems to be the approach outlined in #382, where a separate resource is created specifically for this purpose. What do you think about that?

@hamiltont I'd appreciate your thoughts here too, I'm not super familiar with these features of Keycloak so I may need a bit of guidance.

[hamiltont]

Sure! disclaimer - my golang is at a 'beginner' level, so I'm probably not understanding all of the nuance here, but my keycloak is somewhere around "advanced" so I can help out there

I was indeed thinking the "right" solution would be a different resource type. To my untrained (on golang) eye, this PR looks to have some useful code that could be refactored and used, but the overall approach does look hacky. When I first opened the diff, I was expecting to see code that was a copy of one of the existing "identity provider mapper" source code files, such as resource_keycloak_attribute_importer_identity_provider_mapper.go, into a new file and then modified that new file to remove anything specific to the "Attribute Importer IdP Mapper" and add anything needed for a "Generic IdP Mapper". (Similarly for the test file).

For some context, here are the current source code usages of the "IDP mapper" type:

Note there are four of them, because they correspond to the four builtin "identity provider mappers", as seen here:

#382 is about adding a custom resource type that supports non built in options. For example, I have built some special mappers that utilize google OAuth APIs to collect additional data about a user if they have given consent to access that data. This is not a built-in (and the java code does not extend or subclass anything, it's a completely new implementation of an "identity provider mapper")

Be aware of the common confusion between client mappers and IdP mappers. They are similar in concept (they map data), but an identity provider mapper is for mapping data from an external system (the idp) into the local keycloak system, while a client mapper is for the opposite e.g. mapping data from the local keycloak system into the claims you want to expose to a caller. They are also used at different places in the login flow (Idp mappers are used right after user login flows return from the IdP, while client mappers are typically used when a user auth is totally done and we are about to return the jwt)

[hamiltont]

I do think #434 is a duplicate request as #382 , but it proposed a different solution approach that might be where @bl00mber got some inspiration for this PR. I do think the custom resource approach is better and more maintainable, but I happy defer to @mrparkers on what is considered clean maintainable code in golang

[bl00mber]

Thanks for the reviews!

@mrparkers this way of implementation was selected because generic_keycloak_identity_provider_mapper already had supported filling of other fields of IdentityProviderMapper.
But seems it is irrelevant, since other idp mappers do not need IndentityProviderMapper field to be filled anyways?

I've updated PR, allowing this field to be filled in separate resource instead.

[mrparkers]

I like this approach better, thanks for making those changes! I just have a few comments

[mrparkers]

it looks like you can provide a format string to the identity_provider_mapper attribute? this looks fine, but can this be added to the documentation so it's clear how to use this?

[mrparkers]

Sorry for taking so long to get back to this. Everything here looks good, thanks for submitting this PR!

[inspiredpages]

@mrparkers any idea when this will make it into a release?

[mrparkers]

https://github.com/mrparkers/terraform-provider-keycloak/releases/tag/v3.1.1 - sorry for the delay!

[tomrutsaert]

@bl00mber: Could it be that this change can not handle the keyword "claim" (all lower case) as a param in extra_config?
is "claim" a keyword, that is ignored? I can not find any hints in the code.
Using "Claim" (Uppercase C) does work and that is also what you use in your tests.
But my custom provider mapper uses "claim"
You can easily reproduce the issue by changing "Claim" to "claim" in your tests. ex: testKeycloakCustomIdentityProviderMapper_basic

[bl00mber]

@tomrutsaert does this setting generates camelcase string, or lowercase string?
In keycloak/identity_provider_mapper.go it seemingly is transformed to lowercase string?

[tomrutsaert]

So because of the unmarshal/marshal method and because

claim is part of the IdentityProviderMapperConfig
I can not use "claim" in extra_config in my custom mapper?
So basically I am stuck here?

@mrparkers what do you think?

[mrparkers]

Yeah, the logic for marshalling and unmarshalling structs that have an ExtraConfig is what's causing this. Because of this, none of those JSON attributes in your screenshot can actually be set in the custom identity provider mapper, which I realize is pretty limiting.

This resource is sharing that IdentityProviderMapperConfig model and most of its lifecycle with other identity provider mappers, but I think this should be changed in light of this issue. We'll probably have to make a new model in the keycloak package to account for this.

If you want to open an issue for this @tomrutsaert, I can try to take a look later this week. Otherwise I'm happy to review a PR if you want to take a stab at it too.

[mrparkers]

Actually, someone already opened #571 for this. I'll see if I can make some time sooner rather than later to address this since it's affecting multiple users.