-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cloudstack:8647 LDAP Trust AD and Autoimport #755
Conversation
also added the required dao, table and vo
added check to see if domain is linked to ldap. If yes and the user is member of the group/OU, authenticate and import user.
querying the nested groups only when nested groups are enabled
if an admin username is given to the linkDomainToLdap, added support to import this user User will be imported only if the user is available in the group/ou in ldap and an account with the name doesnt exist in cloudstack. on successful import, accountid will be returned in response.
cloudstack-pull-rats #429 SUCCESS |
cloudstack-pull-analysis #362 ABORTED |
Can someone review please? |
public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName()); | ||
private static final String s_name = "linkdomaintoldapresponse"; | ||
|
||
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.") |
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.
Shouldn't this be mandatory?
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.
yes it should be. will change.
made domainId compulsory in api LinkDomainToLdapCmd used accountServive from BaseCmd in LinkDomainToLdapCmd changed the allowed account type values to 0 and 2
updated the code |
cloudstack-pull-rats #457 SUCCESS |
cloudstack-pull-analysis #390 SUCCESS |
if(result) { | ||
if(user == null) { | ||
// import user to cloudstack | ||
createCloudStackUserAccount(ldapUser, domainId, ldapTrustMapVO.getAccountType()); |
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.
Shouldn't 'user' be assigned the output of this call?
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.
user isnt required after this.
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.
What happens if more than one thread simultaneously calls authenticate for same user for the first time (i.e. both see user is not created)? Will one of the call to createCSUserAccount() fail? Ideally first call should create the account and 2nd should reuse.
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.
The second call to createUserAccount will fail and authentication will fail in the second attempt.
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.
In this case as well, the authn is successful but user sees failure as account creation has failed. The behaviour is not intuitive. If it is difficult to address it in implementation then should be documented as limitation.
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.
In this case as well, the authn is successful but user sees failure as account creation has failed. The behaviour is not intuitive. If it is difficult to address it in implementation then should be documented as limitation.
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.
authentication wont be successful. authentication will fail as createuseraccount will throw exception.
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.
By authn I mean the call to LdapManager.canAuthenticate(). This will be successful in both cases but CS will report authn failure in once case due to account creation failure which is not intuitive. Account creation failure will be passed as authn failure.
But this will be a corner case scenario.
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.
Please open a tracking bug to see if this can be improved to make the outcome more intuitive.
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.
There is no bug here. Its how the current authenticators work. the api output will be improved based on the outcome of CLOUDSTACK-8796
|
||
boolean authenticated = false; | ||
HashSet<ActionOnFailedAuthentication> actionsOnFailedAuthenticaion = new HashSet<ActionOnFailedAuthentication>(); | ||
User.Source userSource = userAccount.getSource(); | ||
User.Source userSource = userAccount != null ? userAccount.getSource(): User.Source.UNKNOWN; |
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.
Looks like ldap user may be authenticated by another authenticator in case user source is unknown. What happens if non-ldap authenticator returns success for ldap user?
Check if it would make sense to have a property on authenticators that says whether lazy user creations is supported by it. And then make use of that to do the validation of userAccount
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.
Ldap imported user will not have the source as unknown. If does, it will continue to check in other authenticators and if they return success, user will be authenticated.
Thats an existing behaviour which is not changed by this PR.
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.
Think about it from the perspective of authenticators. Can a new authenticator break this logic?
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.
I checked that after the discussion yesterday.
Every authenticator checks if a user exists in cloudstack. That responsibility is with the authenticator. In the earlier case, if the user doesnt exist, it throws a NPE on line 2155 which is a recent change. Prior to that change, any cloud provider can write an authenticator which always returns true even when cloudstack account doesnt exist. But, will break in lot of other places with NPEs as the rest of the app assumes logged in user has an account in cloudstack db. I think that needs to be fixed so that anyone can plugin their own user management system without using native cloudstack users.
cloudstack-pull-rats #463 SUCCESS |
@@ -2196,6 +2193,11 @@ private UserAccount getUserAccount(String username, String password, Long domain | |||
s_logger.debug("Unable to authenticate user with username " + username + " in domain " + domainId); | |||
} | |||
|
|||
if (userAccount == null) { | |||
s_logger.warn("Unable to find an user with username " + username + " in domain " + domainId); |
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.
This may also happen if ldap user exists but authn. failed due to incorrect password. The message wouldn't be correct. Is it intentional?
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.
yes.
cloudstack-pull-analysis #395 SUCCESS |
cloudstack-pull-analysis #396 UNSTABLE |
4ea15b6
to
6177bae
Compare
cloudstack-pull-rats #472 SUCCESS |
cloudstack-pull-analysis #405 SUCCESS |
@karuturi LGTM, though one question - once an LDAP group is linked with a cloudstack domain, and then say if I manually create a user in cloudstack's domain first and then say add that same user (or different user but with the same username) what will happen, how does this resolve conflict or log this error? |
users are added based on the username. So, in this case LDAP user wont be added(it will error out). CS user will exist and will continue to work with earlier password using native authentication. |
@koushik-das I created two issue for the open items. I will track these later |
…t fails Incase create useraccount fails with any runtime exception, linkdomaintoldap api shouldnt fail. It just will not return the admin id as it didnt create the account. added test cases to verify this as well.
cloudstack-pull-rats #485 SUCCESS |
cloudstack-pull-analysis #418 SUCCESS |
travis build for the PR is here. https://travis-ci.org/apache/cloudstack/builds/78567814 |
LGTM based on the tracking bugs for addressing some of the issues. |
I am merging this as there are two 👍 Let me know if there are any objections. |
Cloudstack:8647 LDAP Trust AD and AutoimportToday, CloudStack can automatically import LDAP users based on the configuration to a domain or an account. However, any new users in LDAP aren't automatically reflected. The admin has to manually import them again. This feature enables admin to map LDAP group/OU to a CloudStack domain and any changes are reflected in ACS as well. FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/WIP%3A+LDAP%3A+Trust+AD+and+Auto+Import testcases output: ``` ------------------------------------------------------- T E S T S ------------------------------------------------------- Running groovy.org.apache.cloudstack.ldap.NoLdapUserMatchingQueryExceptionSpec Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.216 sec - in groovy.org.apache.cloudstack.ldap.NoLdapUserMatchingQueryExceptionSpec Running groovy.org.apache.cloudstack.ldap.LdapManagerImplSpec log4j:WARN No appenders could be found for logger (org.apache.cloudstack.ldap.LdapManagerImpl). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. using type: using type: null using type: TEST using type: TEST TEST using name: using name: null using accountType: -1 using accountType: 1 using accountType: 3 using accountType: 4 using accountType: 5 using accountType: 6 using accountType: 20000 using accountType: -500000 Tests run: 29, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.387 sec - in groovy.org.apache.cloudstack.ldap.LdapManagerImplSpec Running groovy.org.apache.cloudstack.ldap.LdapListUsersCmdSpec Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.041 sec - in groovy.org.apache.cloudstack.ldap.LdapListUsersCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapAddConfigurationCmdSpec Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.019 sec - in groovy.org.apache.cloudstack.ldap.LdapAddConfigurationCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapUserSpec Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.021 sec - in groovy.org.apache.cloudstack.ldap.LdapUserSpec Running groovy.org.apache.cloudstack.ldap.LdapAuthenticatorSpec Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.082 sec - in groovy.org.apache.cloudstack.ldap.LdapAuthenticatorSpec Running groovy.org.apache.cloudstack.ldap.LdapConfigurationVOSpec Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.004 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationVOSpec Running groovy.org.apache.cloudstack.ldap.OpenLdapUserManagerSpec Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.094 sec - in groovy.org.apache.cloudstack.ldap.OpenLdapUserManagerSpec Running groovy.org.apache.cloudstack.ldap.LdapDeleteConfigurationCmdSpec Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.008 sec - in groovy.org.apache.cloudstack.ldap.LdapDeleteConfigurationCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapUserResponseSpec Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec - in groovy.org.apache.cloudstack.ldap.LdapUserResponseSpec Running groovy.org.apache.cloudstack.ldap.LdapUserManagerFactorySpec Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.027 sec - in groovy.org.apache.cloudstack.ldap.LdapUserManagerFactorySpec Running groovy.org.apache.cloudstack.ldap.ADLdapUserManagerImplSpec Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.012 sec - in groovy.org.apache.cloudstack.ldap.ADLdapUserManagerImplSpec Running groovy.org.apache.cloudstack.ldap.LdapCreateAccountCmdSpec Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.168 sec - in groovy.org.apache.cloudstack.ldap.LdapCreateAccountCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapImportUsersCmdSpec Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.063 sec - in groovy.org.apache.cloudstack.ldap.LdapImportUsersCmdSpec Running groovy.org.apache.cloudstack.ldap.LinkDomainToLdapCmdSpec Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.019 sec - in groovy.org.apache.cloudstack.ldap.LinkDomainToLdapCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapSearchUserCmdSpec Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in groovy.org.apache.cloudstack.ldap.LdapSearchUserCmdSpec Running groovy.org.apache.cloudstack.ldap.LdapListConfigurationCmdSpec Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec - in groovy.org.apache.cloudstack.ldap.LdapListConfigurationCmdSpec Running groovy.org.apache.cloudstack.ldap.NoSuchLdapUserExceptionSpec Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.005 sec - in groovy.org.apache.cloudstack.ldap.NoSuchLdapUserExceptionSpec Running groovy.org.apache.cloudstack.ldap.LdapConfigurationResponseSpec Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationResponseSpec Running groovy.org.apache.cloudstack.ldap.LdapConfigurationSpec asserting for provider configuration: openldap asserting for provider configuration: microsoftad asserting for provider configuration: asserting for provider configuration: asserting for provider configuration: xyz asserting for provider configuration: MicrosoftAd asserting for provider configuration: OpenLdap asserting for provider configuration: MicrosoftAD Tests run: 19, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.053 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationSpec Running groovy.org.apache.cloudstack.ldap.LdapContextFactorySpec Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.099 sec - in groovy.org.apache.cloudstack.ldap.LdapContextFactorySpec Running groovy.org.apache.cloudstack.ldap.LdapConfigurationDaoImplSpec Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.027 sec - in groovy.org.apache.cloudstack.ldap.LdapConfigurationDaoImplSpec Running groovy.org.apache.cloudstack.ldap.LdapUtilsSpec Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec - in groovy.org.apache.cloudstack.ldap.LdapUtilsSpec Results : Tests run: 156, Failures: 0, Errors: 0, Skipped: 0 ``` * pr/755: CLOUDSTACK-8647: linkdomaintoldap shouldnt fail when createuseraccount fails CLOUDSTACK-8647 removed duplicate key in create sql of ldap_trust_map CLOUDSTACK-8647: string formatting CLOUDSTACK-8647: updated with review comments CLOUDSTACK-8647: unittests for LdapAuthenticatorSpec CLOUDSTACK-8647: formatted LdapAuthenticatorSpec CLOUDSTACK-8647: UI for trust AD feature CLOUDSTACK-8647 added unittests for new methods in ldapmanager CLOUDSTACK-8647 unittests for LinkDomainToLdap api command CLOUDSTACK-8647: fixed unittests CLOUDSTACK-8647 support for assigning and admin to linked ldap domain CLOUDSTACK-8647 added nested group enabled config in ldap CLOUDSTACK-8647 added account_type to the linkDomainToLdap API CLOUDSTACK-8647 changed the authentication flow CLOUDSTACK-8647 added new api linkLdapToDomain CLOUDSTACK-8647: added cmd and response class for the new api Signed-off-by: Rajani Karuturi <rajani.karuturi@citrix.com>
issue: Boxing/unboxing to parse a primitive
issue: Boxing/unboxing to parse a primitive
issue: Boxing/unboxing to parse a primitive
Colocar volume ROOT como Destroy quando excluir a VM Closes apache#755 See merge request scclouds/scclouds!316
Today, CloudStack can automatically import LDAP users based on the configuration to a domain or an account. However, any new users in LDAP aren't automatically reflected. The admin has to manually import them again.
This feature enables admin to map LDAP group/OU to a CloudStack domain and any changes are reflected in ACS as well.
FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/WIP%3A+LDAP%3A+Trust+AD+and+Auto+Import
testcases output: