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
Client type service account default type #29037
Client type service account default type #29037
Conversation
@mposolda @graziang @jsorah @vickeybrown , curious what you all think of these changes. This would get rid of the reflection, as talked about here: #28529 (comment). As well as consolidate where client type configurations get applied and validated in the code. I would want to go back and polish before having a true review but wanted to have eyes on this and see if you agree / disagree with the approaches. Thanks! |
b1213ba
to
2b6ea3c
Compare
2b6ea3c
to
b2bea8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMapping
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialAction
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTest
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthentication
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValues
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfiguration
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcr
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSO
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMappingKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialActionKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTestKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthenticationKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValuesKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfigurationKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcrKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSOKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMappingKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialActionKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTestKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthenticationKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValuesKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfigurationKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcrKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSOKeycloak CI - Forms IT (firefox)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickjennings Thanks! As long as the tests are ok, I think that changes are good.
I've added some comments inline (maybe the one about not throwing exception can help with failing tests, but not 100% sure...)
Object nonApplicableValue = getNonApplicableValue(); | ||
// Check if clientType supports the feature. If not, return directly | ||
if (!clientType.isApplicable(propertyName) && !Objects.equals(nonApplicableValue, newValue)) { | ||
throw new ClientTypeException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need this. Isn't it sufficient to just log the warning and return in this case?
If some property is non-applicable, we can ignore the value set on the client. IMO Throwing the clientTypeException seems to be quite invasive and error-prone considering various corner cases around compatibility etc...
import java.util.function.Consumer; | ||
import java.util.function.Supplier; | ||
|
||
enum TypedClientAttribute implements TypedClientAttributeInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
How about renaming TypedClientAttributeInterface
to TypedClientAttribute
and TypedClientAttribute
to TypedClientSimpleAttribute
?
b2bea8b
to
4346e37
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMappingKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialActionKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTestKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthenticationKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValuesKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfigurationKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcrKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSOKeycloak CI - Forms IT (chrome)
org.keycloak.testsuite.federation.ldap.LDAPReadOnlyTest#testReadOnlyWithTOTPEnabled
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMapping
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialAction
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTest
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthentication
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValues
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfiguration
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcr
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSO
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testWithOTPAndRecoveryCodesAtLevel2Keycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testRealmAcrLoaMappingKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDeleteCredentialActionKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testDisableStepupFeatureTestKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#stepupAuthenticationKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#multipleEssentialAcrValuesKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testChangingLoaConditionConfigurationKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithAcrKeycloak CI - Forms IT (firefox)
org.keycloak.testsuite.forms.LevelOfAssuranceFlowTest#testMaxAgeConditionWithSSOKeycloak CI - Forms IT (firefox)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
ac56b2f
to
999cf19
Compare
…-account client type configuration. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…rd client type configurations. Adding overrides for fields in TypeAwareClientModelDelegate required for service-account client type. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
the top level ClientModel fields, the extended attributes through the client_attributes table, and the composable fields on ClientRepresentation. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Validation will be done in the RepresentationToModel methods that are responsible for the ClientRepresentation -> ClientModel create and update static methods. Signed-off-by: Patrick Jennings <pajennin@redhat.com> More updates Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…e, try to get property value from the client. Type aware client model will return non-applicable or default value to keep fields consistent. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…because getter is a boolean and so cannot be null. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…ed before and causing failures in integration tests. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…clients. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
…g update. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Signed-off-by: Patrick Jennings <pajennin@redhat.com>
Signed-off-by: Patrick Jennings <pajennin@redhat.com>
386c12a
to
4e8f474
Compare
…rmining whether to set sane default on create. Signed-off-by: Patrick Jennings <pajennin@redhat.com>
4e8f474
to
188e36d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickjennings Thanks!
Changes requested as part of #29198.
Removing reflection used in augmentation of the client representation during create and update. This is replaced by utilizing the
TypeAwareClientModelDelegate
to catch any ClientTypeExceptions that might be thrown on fields that are non-applicable or read-only.Static methods
updateClient
andcreateClient
ofRepresentationToModel
will handle updating the model from the client representation and consolidating validations error to report all failures of client type fields. Removed duplicate code used to update the properties in these two methods.Created enums for well known client type attributes, in
TypedClientAttribute
andTypedClientExtendedAttribute
. These also define non-applicable default values for each of the properties.With this, you can create a service account with all relevant fields by only specifying the type: