-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fixed SMB Login Issues #100
Conversation
I'm not able to fix the DCO error, as git (current version) doesn't recognizes the argument |
@@ -39,30 +39,28 @@ public function __construct($host) { | |||
private function tryAuthentication($uid, $password) { | |||
$uidEscaped = escapeshellarg($uid); | |||
$password = escapeshellarg($password); | |||
$command = self::SMBCLIENT.' '.escapeshellarg('//' . $this->host . '/dummy').' -U'.$uidEscaped.'%'.$password; | |||
$command = self::SMBCLIENT.' '.escapeshellarg('//' . $this->host . '/dummy').' -U '.$uidEscaped.'%'.$password; |
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 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.
where can you see her that the space is needed?
maybe the safer option would be to use --user=username
?
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.
where can you see her that the space is needed?
Added a link with proper information 🙈
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.
okay thank you very much, seems legit :)
You have a point but the code is old and there are no tests. I wouldn't touch that ;) |
You can tell me if I can help with testing. I'm really not a fan of not touching code, because it has been there a long time. (For me that's even more reason to touch it :D) |
There is a difference between fixing a bug and refactoring code. You can fix a bug without refactoring code and refactor code without fixing bugs. Is the refactoring of the parse response logic required to fix the bug? No. Your new parse response logic looks fine and is better than before but there is always a chance to introduce new bugs. Usually their are tests to ensure that a method does not break. Unfortunately there are no tests and the risk is higher. Adding unit tests to this logic is quite hard with the current code. We could move the logic into a method and write some tests. I don't think its possible to mock I would suggest to fix the bug and submit another pr with your refactoring. In the end it's up to @violoncelloCH because he is the maintainer of the app ;) |
Thank you for the clarification. I'll see with @violoncelloCH and next time I'll create 2 PR's. (I'm still new to this :D) Have a nice week. |
hi @melvin-suter |
regarding the tests: if you have some experience with unit testing and would be willing to help, I would appreciated it if you could take a look at those... there are a few tests in |
fc339a9
to
bc46dac
Compare
So. I was able to signoff after installing git2u and I reverted back to the original if/else version instead of the switch, so this PR only contains the space after I'm going to add another PR with the new switch version, so it is done the right way around. :D |
@melvin-suter thanks for splitting this into separate PRs! |
3796890
to
d9e13db
Compare
Signed-off-by: Melvin Suter <github@damon.ch>
d9e13db
to
b1032c5
Compare
I rebased and squashed your commits to not contain reverted changes... |
Fixes:
We had problems with logging in some users over SMB. The error:
The first, expected to be "incorrect", login try was successful. (Because of a bad formatting in the smbclient command.) I've added the needed space after the
-U
argument.Additional changes:
I rewrote the multiple
if
/elseif
/else
's &goto
's to a easier to maintain/more professional version. (one if + switch)Further Proposals:
Re-evaluate if the
NT_STATUS_BAD_NETWORK_NAME
catch is really needed or at least add a check for successful login, instead of assuming the login was successful on that error.