Skip to content
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

Adds openid-user-attribute protocol mapper #16

Merged

Conversation

@alexashley
Copy link
Collaborator

commented Oct 5, 2018

No description provided.

@mrparkers

This comment has been minimized.

Copy link
Owner

commented Oct 5, 2018

I fixed the CircleCI config so it builds PRs submitted via forks. Closing and re-opening to trigger a build.

@mrparkers mrparkers closed this Oct 5, 2018
@mrparkers mrparkers reopened this Oct 5, 2018
}

return fmt.Sprintf("/realms/%s/%s/%s/protocol-mappers/models", realmId, parentResourceId, parentResourcePath)
}

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 5, 2018

Owner

Nice work supporting both client and client-scope 👍

Name string `json:"name"`
Protocol string `json:"protocol"`
ProtocolMapper string `json:"protocolMapper"`
Config map[string]interface{} `json:"config"`

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 5, 2018

Owner

Why is the val of type interface{}? It looks like it's always a string in the API responses I'm seeing.

This comment has been minimized.

Copy link
@alexashley

alexashley Oct 5, 2018

Author Collaborator

Good catch, I'll change it. When I wrote the struct definition I assumed that the boolean fields were real bools and not strings.

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 5, 2018

Owner

Haha, yeah you probably should not assume that. The Keycloak API is all over the place with stuff like that.

clientId := data.Get("client_id")
clientScopeId := data.Get("client_scope_id")

if clientId == "" && clientScopeId == "" {

This comment has been minimized.

Copy link
@alexashley

alexashley Oct 9, 2018

Author Collaborator

Do you know of a better way to do this? I have ConflictsWith set for both, but I don't know of a way to validate that one or the other must be set.

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 9, 2018

Owner

No, that is probably the best way to do it. For models that require validation that depend on other resource attributes or some state on the server, I've been creating a Validate() method that I use before creating the model on the server, since otherwise the feedback isn't very helpful.

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

Thoughts on pulling out the validation code to a Validate function for consistency? Other resources do this as well.

@alexashley alexashley changed the title [WIP] Adds openid-user-attribute protocol mapper Adds openid-user-attribute protocol mapper Oct 16, 2018
@alexashley

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 16, 2018

This is ready for review

// import a mapper tied to a client:
// {{realmId}}/client/{{clientId}}/{{protocolMapperId}}
// or a client scope:
// {{realmId}}/client-scope/{{clientScopeId}}/protocolMapperId

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

{{protocolMapperId}}

}
}

func mapFromDataToOpenIdUserAttributeProtocolMapper(data *schema.ResourceData) *keycloak.OpenIdUserAttributeProtocolMapper {

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

I've calling these functions something like getOpenIdUserAttributeProtocolMapperFromData but I think I like your name better, since the term get can mean a lot of different things here. I might revisit these functions in the future and rename them.

clientId := data.Get("client_id")
clientScopeId := data.Get("client_scope_id")

if clientId == "" && clientScopeId == "" {

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

Thoughts on pulling out the validation code to a Validate function for consistency? Other resources do this as well.

openIdUserAttributeMapper, err = keycloakClient.GetOpenIdUserAttributeProtocolMapperForClient(realmId, clientId, data.Id())
} else {
openIdUserAttributeMapper, err = keycloakClient.GetOpenIdUserAttributeProtocolMapperForClientScope(realmId, clientScopeId, data.Id())
}

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

I wonder if this would look better if there was just a single function, keycloakClient.GetOpenIdUserAttributeProtocolMapper, that took both client id and client scope id, and figured out which of the two this mapper belongs to based on which string is empty. I'd rather have the code in the keycloak package do stuff like this.

claim_name = "bar"
claim_value_type = "%s"
}
`, realmName, mapperName, invalidClaimValueType)

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 16, 2018

Owner

I don't have a good reason for this, but conventionally this is pulled out into a function that lives near the bottom of this file.

@alexashley alexashley closed this Oct 17, 2018
@alexashley alexashley reopened this Oct 17, 2018
@alexashley

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 17, 2018

@mrparkers Mind taking another look at this pr? I think I've addressed everything

//
//func (keycloakClient *KeycloakClient) GetOpenIdUserAttributeProtocolMapperForClientScope(realmId, clientScopeId, mapperId string) (*OpenIdUserAttributeProtocolMapper, error) {
// return keycloakClient.getOpenIdUserAttributeProtocolMapper(realmId, "", clientScopeId, mapperId)
//}

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 17, 2018

Owner

Forgot these


func testAccKeycloakOpenIdUserAttributeProtocolMapperDestroy() resource.TestCheckFunc {
return func(state *terraform.State) error {

This comment has been minimized.

Copy link
@mrparkers

mrparkers Oct 17, 2018

Owner

Can you remove the extra whitespace here?

@mrparkers

This comment has been minimized.

Copy link
Owner

commented Oct 17, 2018

LGTM after those minor comments

alexashley added 2 commits Oct 17, 2018
@mrparkers mrparkers merged commit 3f8f95f into mrparkers:master Oct 17, 2018
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@alexashley alexashley deleted the alexashley:openid-user-attribute-protocol-mapper branch Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.