hedgedoc / cli

A tiny CLI for HedgeDoc
GNU Affero General Public License v3.0
150 stars 37 forks source link

Refactor script a bit #19

Closed fbartels closed 5 years ago

fbartels commented 5 years ago

Hi,

I have spent some time refactoring codimd a while ago. I have not rebased this on the current version, but since I do not have time at the moment to end my refactoring I wanted to open a pr now.

I do have a problem with the code at the moment, since logins (for my ldap backend) seem to fail.

pirate commented 5 years ago

I don't have LDAP set up, but maybe @SISheogorath can test this branch to see if it works against his LDAP-enabled CodiMD server. I'm eager to merge this since there are a lot of improvements, I just don't want to break LDAP for existing users if it's still untested/buggy with these changes.

SISheogorath commented 5 years ago

Seems like a simple copy & paste mistake:

diff --git a/bin/codimd b/bin/codimd
index 53b757e..31c927e 100755
--- a/bin/codimd
+++ b/bin/codimd
@@ -94,7 +94,7 @@ function authenticate_email() {
 function authenticate_ldap() {
     curl -c $CODIMD_COOKIES_FILE \
         -XPOST \
-        --data-urlencode "email=$1" --data-urlencode "password=$2" \
+        --data-urlencode "username=$1" --data-urlencode "password=$2" \
         $CODIMD_SERVER/auth/ldap &>/dev/null
 }
SISheogorath commented 5 years ago

Also just as an idea, maybe add something like CODIMD_DEBUG which triggers set -x ;)

fbartels commented 5 years ago

Seems like a simple copy & paste mistake

Hmm.. Looking at the diff this must have been broken before as well. I'll give it a quick try and push a fix if this is indeed the culprit.

SISheogorath commented 5 years ago

:+1: Great work! Really look forward to have this merged. I'll add some CI stack later this weekend and during the next week.

SISheogorath commented 5 years ago

I'll leave this to @pirate now :) LGTM

fbartels commented 5 years ago

@pirate I'd say it is good for now.