nextcloud / user_external

👥 External user authentication methods like IMAP, SMB and FTP
https://apps.nextcloud.com/apps/user_external
108 stars 64 forks source link

Fixed SMB Login Issues #100

Closed melvin-suter closed 4 years ago

melvin-suter commented 5 years ago

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.

melvin-suter commented 5 years ago

I'm not able to fix the DCO error, as git (current version) doesn't recognizes the argument --signoff.

kesselb commented 5 years ago

I rewrote the multiple if/elseif/else's & goto's to a easier to maintain/more professional version. (one if + switch)

You have a point but the code is old and there are no tests. I wouldn't touch that ;)

melvin-suter commented 5 years ago

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)

kesselb commented 5 years ago

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 exec so there is no other way.

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 ;)

melvin-suter commented 5 years ago

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.

violoncelloCH commented 5 years ago

hi @melvin-suter first of all: thank you very much for your contribution! as @kesselb already said, I would prefer you would split up things like this into multiple PR's so they can be discussed and merged separately... would you mind doing this? you could also add the signoff then... (-s in your commit command should do the trick https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s or you can also add your sign-off manually of course... just check the last line of e.g. one of my commits for the syntax)

violoncelloCH commented 5 years ago

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 tests/ but I didn't get them to work... the goal would be to make add them to the travis CI - see #40

melvin-suter commented 4 years ago

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 -U.

I'm going to add another PR with the new switch version, so it is done the right way around. :D If I got the time, I'll try to look into the testing-thing.

violoncelloCH commented 4 years ago

@melvin-suter thanks for splitting this into separate PRs!

violoncelloCH commented 4 years ago

I rebased and squashed your commits to not contain reverted changes...