CA-386281 CIFS username can be omitted in ISO SR #663
Conversation
ISOSR falling over when no username is given is a regression that looks to have been introduced in commit cbd7226 when the use of a credentials file was removed. The ISOSR unit tests were already covered cases of not having credentials - as evinced by having "guest" in the expected mount options. However, they were doing this in a rather confused way that hid the bug by supplying a username regardless. Now only the credentials tests do this. Signed-off-by: Robin Newton <[email protected]>
| username, domain = ( | ||
| cifutils.splitDomainAndUsername(self.dconf['username']) | ||
| ) | ||
|
|
||
| if domain: |
There was a problem hiding this comment.
Why not just move this up into the else clause ?
There was a problem hiding this comment.
That's what I thought I did. Is the diff somehow showing back-to-front?
Oh, you're talking about the if domain. No, that has to be kept, because domain can come back None from splitDomainAndUsername.
There was a problem hiding this comment.
It might have been better if the stuff with the SMB version didn't come between the conditional blocks. But the unit tests are sensitive to the order in which options come - I toyed with making them less sensitive, but it seems like quite a lot of faff.
There was a problem hiding this comment.
That's what I thought I did. Is the diff somehow showing back-to-front?Oh, you're talking about theif domain. No, that has to be kept, because domain can come backNonefromsplitDomainAndUsername.
Well, yes, but we know that there can never be a domain in the positive case of the if so making the else be
else:
_, domain = cifutils.splitDomainAndUsername(self.dconf['username'])
if domain:
options.append('domain=' + domain)would then remove the need for the domain=None in the guest handling side of the branch.
Maybe we should do this as a future improvement to avoid the sensitivity of the UTs to the order of the provided options.
Yes. In XenIA-Pre I found the SMB path for a collection of Windows ISOs that can be used without authenticating. I didn't test the case where authentication is required but no username is given - I got a bit bogged down try to set up an SMB server - so I'm assuming it would fail in the same way that it would if invalid credentials were given. |
ISOSR falling over when no username is given is a regression that looks to have been introduced in commit
cbd7226 when the use of a credentials file was removed.
The ISOSR unit tests were already covered cases of not having credentials - as evinced by having "guest" in the expected mount options. However, they were doing this in a rather confused way that hid the bug by supplying a username regardless. Now only the credentials tests do this.