Potential for inconsistent state?

[brightghost]

I've been taking a look at this project, and I'm not sure if the error handling is adequate for such a sensitive operation. For example, what will be the result if the password update is attempted but the Computer AD object is not writable? It looks like this block at PWChange:22 where the real work takes place:

do {
    // Pull Local Administrator Record
    let local_node = try ODNode.init(session: ODSession.default(), type: UInt32(kODNodeTypeLocalNodes))
    let local_admin_change = try local_node.record(withRecordType: kODRecordTypeUsers, name: local_admin, attributes: nil)
    // Change the password for the account
    try local_admin_change.changePassword(nil, toPassword: password)
    // Set out nex expiration date in a variable x days from our
    // configuration variable
    let new_ad_exp_date = time_conversion(time_type: "windows", exp_time: nil, exp_days: exp_days) as! String
    // Format Expiration Date
    let print_exp_date = time_conversion(time_type: "epoch", exp_time: new_ad_exp_date, exp_days: nil) as! Date
    let formatted_new_exp_date = dateFormatter.string(from: print_exp_date)
    // Change the password in Active Directory
    _ = ad_tools(computer_record: computer_record, tool: "Set Password", password: password, new_ad_exp_date: new_ad_exp_date)
    laps_log.print("Password change has been completed for local admin " + local_admin + ". New expiration date is " + formatted_new_exp_date, .info)
} catch {
    laps_log.print("Unable to connect to local directory or change password. Exiting...", .error)
    exit(1)
}

As I read it, local_admin_change.changePassword() is called before any attempt is made to verify the AD Computer object can be written to, which only takes place in the call to ad_tools(computer_record: computer_record, tool: "Set Password", password: password, new_ad_exp_date: new_ad_exp_date). The message logged in the catch block suggests the operation would be aborted in case of an exception, but if the Computer record can't be updated, won't this in fact leave you with a new password on the local admin, which is not recorded anywhere?

I'm not particularly familiar with Swift, so apologies if there's something to the flow control I'm missing here. Or is this check performed earlier in another function? I see you can potentially exit(1) at ADTools:31 , but it seems this will just verify a record was found, not that it's writable.

[joshua-d-miller]

So after reading through your detailed question (which thank you btw) I think at least from the way I designed it which maybe I missed something that the password won't even be generated if you can't connect to Active Directory. The one instance I could see this being an issue is if the machine is bound but is acting like it isn't bound. Upon testing this though you are unable to pull any information from AD in this state which means the program would stop during the connection to AD in the ADTools.

[patriciaho]

I've been thinking about this flow as well. Our situation is that a machine randomly connects to one of a handful of DCs, some are read-only. So in our case, it would be best if passwords weren't changed locally unless it's confirmed that the new password and expiry date have successfully been written to AD. Checking if the machine is bound in this case would not be sufficient.

[joshua-d-miller]

@patriciaho I'm thinking what we need to do then is maybe add the ability to specify the domain controller to connect to if possible that way you can make sure the record is writable.

[joshua-d-miller]

Please try the new version that will write the password first to Active Directory before writing it locally. Let me know if this resolves this issue.

[joshua-d-miller]

I believe this has been resolved but if it hasn't please let me know!