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

Add Prosody XMPP MySQL DB authentication #37

Closed sebastiansterk closed 5 years ago

sebastiansterk commented 5 years ago

This pull request contains an implementation for authentication against a Prosody XMPP MySQL database. The user can login to a nextcloud instance with his XMPP ID + password.

violoncelloCH commented 5 years ago

Hi @sebastiansterk thank you for your contribution! However I've got some considerations first:

so what are your thoughts and arguments? :smile:

sebastiansterk commented 5 years ago

1) this is only for prosody's mysql database 2) it is specific, as mentioned this is an authentication against a prosody mysql database incl. hmac_sha1 check - you just need a db user that has access to the prosody db. how many users and instances? Idk, in the xmpp ecosystem there are two big xmpp servers: ejabberd and prosody. Prosody admins can use this plugin so all users can use their xmpp account to use the nextcloud instance. A authentication against the xmpp service itself would be better. This is just a very simple and minimalistic solution w/o huge xmpp authentication classes. 3) yes

violoncelloCH commented 5 years ago

@sebastiansterk thank you for explaining! @nextcloud/user_external @MorrisJobke @nickvergessen what do you think?

MorrisJobke commented 5 years ago

I can fully understand both sides here, but this should be totally up to the maintainer of the app. I don't have a real opinion here nor would I force anybody here to some way.

There is also the user_sql app that also has similar capabilities.

violoncelloCH commented 5 years ago

So I think if @sebastiansterk is willing to help testing new releases I'm fine with merging this... :)

violoncelloCH commented 5 years ago

thank you @sebastiansterk please review @nextcloud/user_external

violoncelloCH commented 5 years ago

@sebastiansterk there are some conflicts... could you rebase your work on current master?

sebastiansterk commented 5 years ago

@sebastiansterk there are some conflicts... could you rebase your work on current master?

Done, but Travis build failed because of: The command "sudo apt-get -qq update" failed and exited with 100 during . Is there a way to run that job again?

violoncelloCH commented 5 years ago

Is there a way to run that job again?

restarted the job...

a more elegant way than merging the master branch into the PR branch would have been to use git rebase... could you revert the merge commit and do a rebase in command line, so we don't have this unwanted extra commit in the git history?

violoncelloCH commented 5 years ago

thank you @sebastiansterk

sebastiansterk commented 5 years ago

Is there a date planned when it will be merged?

violoncelloCH commented 5 years ago

I would have liked to have a review from someone who has more php experience than me, because I also can't test this without a Prosody setup. Maybe @ChristophWurst would have the time to take a look at this as we already talked about this during the Contributor Week?

violoncelloCH commented 5 years ago

ahh, and again, it would be nice if you @sebastiansterk could resolve conflicts with a git rebase instead of merging master into this branch.

violoncelloCH commented 5 years ago

thank you @ChristophWurst !

violoncelloCH commented 5 years ago

thank you @sebastiansterk for the code styling! :rocket: what about the merge commit?

ChristophWurst commented 5 years ago

I would suggest to squash the commits into one to keep the change atomic

violoncelloCH commented 5 years ago

@ChristophWurst ahh, that's an idea however Github tells me that squashing is not enabled for this repository... can you do this?

ChristophWurst commented 5 years ago

Just do it locally and force push the changes back :wink:

violoncelloCH commented 5 years ago

huh, never done this... how does this work; I can't push to this PR's branch, because it's not in the nextcloud repo...? so do I need to push to an other branch and then merge this one?

ChristophWurst commented 5 years ago

You should be able to. Just add the fork as new remote.

ChristophWurst commented 5 years ago

Something like

git remote add sebastiansterk git@github.com:sebastiansterk/user_external.git
... usual rebase magic ...
git push sebastiansterk/master --force
violoncelloCH commented 5 years ago

yay, it worked! :rocket: thank you @ChristophWurst for the help! and thank you @sebastiansterk for the contribution!

violoncelloCH commented 5 years ago

@sebastiansterk as you are in the Nextcloud organisation since a few days :rocket: I also invited you to the user_external group/team so it's easier to keep you up to date about what's happening in user_external :) It also means that you can now review PRs and create branches in Nextclouds repositories directly (for easier collaboration)... I would also love to see future contributions :)

sebastiansterk commented 5 years ago

Thank you @violoncelloCH :)