Repository navigation
Step up authentication for saml - #44185
Conversation
4db15e7 to
f194bfb
Compare
|
OK, I have implemented the realm map. Finally it is the following:
The rest is more or less the same but I have extended testing and polished some corner cases. For me it's OK and it's fully functional (see the tests) and cover everything. The only missing piece is the documentation. Maybe I would manage it as a follow-up. |
f194bfb to
ab827d4
Compare
ab827d4 to
e07ceb9
Compare
|
Moving to ready for review. The JS CI will be fixed by #44883. |
|
Rebased again. 😄 |
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.oauth.OIDCProtocolMappersTest#testTokenScriptMappingorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserGroupRoleToAttributeMappersScopedClientNotSetorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeUserOneGroupMultivalueNoAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserGroupRoleToAttributeMappersNotScopedOtherApporg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeOneGroupMultiValueNoAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testAddressMappingWithAdditionalMapperorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testClaimFromUserPropertyMapperWithDefaultProfileScopeorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testSingleValuedRoleMappingorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeTwoGroupHierarchyMultiValueNoAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeTwoGroupHierarchyNoMultivalueNoAggregateFromParentorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testRoleMapperWithRoleInheritedFromMoreGroupsorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testTokenMappingorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserRolesMovedFromAccessTokenPropertiesorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testAddressMappingWithoutPresentAddressorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserRoleToAttributeMappersorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testAddressMappingWithAdditionalMapperReversedOrderorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testTokenPropertiesMappingorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeOneGroupMultiValueAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testHardcodeRoleAllorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testStaticScopeUsingDynamicScopeFormatPrefixedWithScopeAsOptionalScopeorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeTwoGroupMultiValueNoAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserGroupRoleToAttributeMappersorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserGroupRoleToAttributeMappersScopedorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testUserGroupRoleToAttributeMappersScopedWithDifferentClientorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testGroupAttributeUserOneGroupMultivalueAggregateorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testRoleNameMapperAllorg.keycloak.testsuite.oauth.OIDCProtocolMappersTest#testNullOrEmptyTokenMapping |
|
|
||
| :tech_feature_name: Step-up Authentication for SAML | ||
| :tech_feature_id: step-up-authentication-saml | ||
| include::../../topics/templates/techpreview.adoc[] |
There was a problem hiding this comment.
This adds the "preview" warning in the documentation, however the feature is enabled as "experimental" in this PR. It seems the options are:
-
Mark the feature as "preview" instead of "experimental" . Which IMO is OK as it is small feature and isolated
-
Update this documentation to mark as feature as "Experimental" . It should be either some "hand-written warning" or a template for experimental features (Something like
include::../../topics/templates/techexperimental.adoc).
My vote is (1) ATM .
Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
Polishing of the feature. Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
52bb8b3 to
9ad0a9a
Compare
|
@mposolda Rebased. Promoted to preview. I have added the release note and also prepared the migration to include the client scope when supported. |
Closes keycloak#10155 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes #10155
This is the initial idea for SAML step-up authentication. It's incomplete because I have not done the mapping at realm level. This PR just uses the mapping at client level. In saml we use the authentication context class ref (which is an uri, for example
urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport) and I suppose it will be different to the OIDC name. So in the client is the same map{"uri":"loa",...}but we have to think something for the realm.What do you prefer?
saml.acr.loa.mapor similar), but it's very convoluted for the admins.[{"acr":"silver","uri":"urn:oasis:names:...","loa":"1"},...]. But this needs to convert the current realm values on the fly or at upgrade.Suggestions?
@mposolda The spec says that requesting an incorrect authn context returns an error in saml protocol (second status). So I need to implement a
sendProtocolErrorin the bindings that uses the protocol. But I discovered theinResponseTowas not properly set for errors. I added this ID in the client data. Is this OK? Or better we just use theinResponseToin the methods that the auth session is available?Draft for the moment because, we need to manage mapping at realm level before.