Repository navigation
Conversation
This adds an option to enable new imported users to Active Directory right after registration. The purpose of this is to have user accounts enabled on Active Directory before the user has created a password. This is accomplished by: - creating a random `unicodePwd` attribute - applying 512 to the `userAccountControl` attribute The accompanying change to the UI is committed to its repo. Closes keycloak#16849
Gives the users the option to enable new users on Active Directory. This is the accompanying UI change to the PR submitted to the server repo: keycloak/keycloak#16980
|
Hi @stianst. Do I need to do anything here (re. failed tests)? |
|
Hi! Any development here? |
|
@oculos - I've been reading up on the discussion, and found the following sentence from you:
So I wonder if you set the password only to avoid the update password required situation? What would happen if you wouldn't set a password, and we could somehow prevent Keycloak to ask for the password? |
Thanks a lot for your question @ahus1 . When I wrote that post initially, that was what happened, that is, neither the As I investigated further, I realized that setting We thought, ok, then we also set That's why I needed to write this PR, which is basically setting the Some of the code I sent could be slightly changed, like instead of using |
|
@oculos - I'm re-adding the reviewers. Not sure how you removed them, but you should try not to remove them. There can be more than one reviewer on an issue. And as both frontend and backend are involved, there will be more than one reviewer. |
Thanks. I have no idea how that happened. I just commit the changes that were requested and asked again for review. I’ll ask first next time. |
There was a problem hiding this comment.
@oculos - I see that the user is create with a random password which the user doesn't know about. I assume this would be safe as it is random, still it would make it hard for people to understand that this user has password that the person doesn't know about.
Would there be a different setup in AD for a user without a random password set - even if that would require an additional check / flag in Keycloak?
I see that you merged the main branch into this branch; maybe this triggered it. Rebasing your work branch might work better. I see that you're using the For the next PR, I'd suggest creating a feature branch in your repo first before creating a PR. |
I need to check, but the truth is that I don’t know. If you see the “UserAccessControl.Java” class (and the LDAPConstants) , there might be something there. That said, on the plain LDAP solution this is done without that kind of need. If one needs the user to set a known password, one would simply add the create password form on the authentication flow. |
|
Hi - any news on this? We really could benefit from getting this merged as we have a project that depends highly on this feature. |
There was a problem hiding this comment.
The changes to the UI here look good to me.
3 flaky tests on run #6885 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Clients test > Roles tab test > Should search existing client role |
Output
Screenshots
|
|
clients_saml_test.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Clients SAML tests > SAML settings tab > should check SAML capabilities |
Output
Screenshots
|
|
realm_test.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Realm tests > should create Test realm |
Output
Screenshots
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
There was a problem hiding this comment.
@oculos Thanks for the PR. I am sorry for the late review.
To me, it would make more sense to add new option to MSAD User Account Mapper rather than directly to LDAP provider. As you pointed, the option is valid only in MSAD and not for any other LDAP vendor. Also the "LDAP provider" itself already has quite a lot options on it. Finally, MSAD mapper is used to integrate with MSAD attributes like userAccountControls and pwdLastSet etc, so logically, it would makes more sense to add the option directly to it.
I think this would mean that this PR won't require much changes to admin console itself. It seems the error message ldapErrorEnableAdUserAvalaibleForAdOnly won't be needed. The error for writable provider can be done in MSADUserAccountControlStorageMapperFactory.validateConfiguration method (method can be overriden from parent class LDAPStorageMapperFactory).
On the other hand, it can be nice to have some small update to the docs section https://www.keycloak.org/docs/latest/server_admin/index.html#_ldap_mappers where MSAD User Account Mapper is documented and add some small note about default behaviour and about this new option to the mapper? See the file ldap.adoc in the project.
Also automated test can be nice. There is LDAPMSADMapperTest, which has most of LDAP tests for MSAD. Maybe new method can be added to it to test new option? Running the tests with MSAD might be tricky and we don't have good documented for it yet... I think you may need to update file https://github.com/keycloak/keycloak/tree/main/testsuite/integration-arquillian/tests/base/src/test/resources/ldap/ldap-connection.properties for your environment and use something like:
idm.test.ldap.connection.url=ldaps://my-server.org:636
idm.test.ldap.base.dn=ou\=Mposolda,dc\=keycloak,dc\=org
idm.test.ldap.roles.dn.suffix=ou\=Roles,ou\=Mposolda,dc\=keycloak,dc=org
idm.test.ldap.group.dn.suffix=ou\=Groups,ou\=Mposolda,dc=keycloak,dc\=org
idm.test.ldap.user.dn.suffix=ou\=People2,ou\=Mposolda,dc\=keycloak,dc\=org
idm.test.ldap.agent.dn.suffix=ou\=Agent,ou\=Mposolda,dc\=keycloak,dc\=org
idm.test.ldap.start.embedded.ldap.server=false
idm.test.ldap.bind.dn=MY\\Administrator
idm.test.ldap.bind.credential=MyPassWord1
idm.test.ldap.cleanup.dn=ou\=Mposolda,dc\=keycloak,dc\=org
idm.test.ldap.connection.pooling=true
idm.test.ldap.pagination=true
idm.test.ldap.batch.size.for.sync=3
idm.test.ldap.vendor=ad
#idm.test.ldap.username.ldap.attribute=sAMAccountName
idm.test.ldap.username.ldap.attribute=cn
idm.test.ldap.user.object.classes=person,organizationalPerson,user
idm.test.ldap.user.account.controls.after.password.update=true
idm.test.ldap.sleepTime=10000
(update properties according your server environment. I think the referenced OUs should exists or you can use the existing file users.ldif from this directory to import them, but you may need to adjust to your environment). And then after build the project, run the test with something like:
cd testsuite/integration-arquillian/tests/base
mvn clean install -Dtest=LDAPMSADMapperTest
Hope this helps
WDYT?
|
@mposolda Thanks for your feedback. I also agree that it would make more sense to add this to a mapper. However, since mappers are applied when creating a user, I didn't see any opportunity there to add a post-creation action. If you still believe it should be done that way, could you point me to the right direction? I am not super familiar with testing, so I am not sure I'm qualified to do that. |
|
@oculos Maybe BTV. What is your use-case for creating user? Are you using user registration or admin console or both? With user registration, I think that you don't need this functionality because password is always provided by user at the time of user registration? On the other hand with admin console, the password is not provided right away, but when you add password to the user right after you create him in admin console, you should achieve same effect? Also the admin REST API for creating user allows to send password credential as part of the user's JSON, so it is possible to create user and update his password in exactly same transaction (This requires admin REST API directly as admin console doesn't support this). Regarding the test: I afraid that automated test is needed when creating new feature with the additional switch. Hopefully the instructions above can help with it (on the other hand, I tend to agree that running automated test against your MSAD LDAP might be a pain...) |
I will see if I can test this at sometime next week, but I was under the impression that this is not called AFTER the first LDAP call (ie., the one that actually creates the user) was issued. The point here is that LDAP against AD doesn't work to set the enabled attribute while creating a user. It needs to be a subsequent LDAP query.
None of those, actually :) We use an external IdP for authentication, so the user is provisioned on the fly on our LDAP User Federation (AD) storage. The use case is that we have an application that requires an existing AD-user. The service supports SAML, and so does our IdP, but the underlying service requires the user to exist on AD, therefore we must have Keycloak in the middle as a just-in-time user provisioning mechanism. The problem, as I described before, is that, as of now, Keycloak does not allow an automatic user creation on AD where the user is enabled by default, so when the user is directed to the service, he/she doesn't have access to the resources.
I'll try to do it again. It's been a almost half a year since I worked with this, so I guess now I need to remember everything about setting up the development environment and so on... |
Thanks for clarifying! I was wondering if IDP can be also one of the use-cases... :-) I suggest to name the switch in MSAD mapper to something like
|
|
Hi,
I won’t be able to work on this PR over the following 8 weeks.
I am also not super familiar with the testing procedures mentioned, so I think I actually give up this PR.
… On 3 Jul 2023, at 13:58, Jon Koops ***@***.***> wrote:
@oculos <https://github.com/oculos> are you still interested in landing this PR? Is @mposolda <https://github.com/mposolda>'s feedback here enough to proceed?
—
Reply to this email directly, view it on GitHub <#16980 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABT2F44EOKRZC3BVQ57Q2ULXOKXX7ANCNFSM6AAAAAAUWYOZAE>.
You are receiving this because you were mentioned.
|
OK. In that case I'm just going to close this PR and its issue. We can pick it back up again if there is future interest. |
This adds an option to enable new imported users to Active Directory right after registration.
The purpose of this is to have user accounts enabled on Active Directory before the user has created a password.
This is accomplished by:
unicodePwdattributeuserAccountControlattributeThe accompanying change to the UI is committed to its repo (https://github.com/keycloak/keycloak-ui/pull/4362)
Closes #16849