tykeal / ep_ldapauth

(Up for adoption) LDAP authentication plugin for Etherpad-lite
GNU General Public License v2.0
25 stars 20 forks source link

public read only #4

Closed derveloper closed 10 years ago

derveloper commented 10 years ago

Hi,

we need to have public read-only pads, i have changed the plugin to enable this. see https://github.com/piratenfraktion-nrw/ep_ldapauth/commit/f1186ce73f4dc9aee229778bdb82061af3d6742f for that. basically it looks for the read-only link and permits the request for everyone if it is the read-only link. i hope i have not done any shit with my change :)

my question is, is this wanted for this plugin in general? if so, i would send you a pull request..

tykeal commented 10 years ago

How exactly are you enabling read-only pads? Admittedly, I don't know Etherpad that well but I haven't seen any option for a read-only pad.

The closest I've seen is the public_view plugin (https://npmjs.org/package/ep_public_view) which makes a non-editable (with edit link) view of pads available at /public/

derveloper commented 10 years ago

within the "Share and Embed Pad" view, there is a checkbox for read-only pads.

maybe it would be an good idea to have a setting for that, i will add this tomorrow, so nothing brakes current behaviour.

tykeal commented 10 years ago

Ah, I see. I hadn't played with that feature.

Second question. I looked over the change and you've added ; to the end of a couple of the functions. Was there a reason for that? Again, I admit, I'm not that proficient with JS and especially not for node.js ;) I stuck my hand into writing this because I needed it more than anything else. I'm mostly a perl hack.

tykeal commented 10 years ago

Oh, one other thing. I think that enabling something like this would be good, yes, however, there needs to be a way to keep the entire system closed up if need be (essentially the current configuration). So perhaps adding an extra flag into the configuration object to enable anonymous view via the read-only or /public/ paths. That should probably enable anonymous access to / as well since that isn't currently available either.

Just an idea for you.

derveloper commented 10 years ago

The semicolons are there because the functions are defined in "functional style" e.g. var foo = function() {...}; The semicolon is not mandatory, but i tend to insert it there because of my habits :) There is something in the standard about this http://www.ecma-international.org/ecma-262/5.1/#sec-7.9

So back to the topic, i've added an new option "anonymousReadonly" to the plugin, so no default behaviour will brake (hopefully :)) see the commit here https://github.com/piratenfraktion-nrw/ep_ldapauth/commit/8c347d2ed8d462ba87a9f134393bcd077043b430

I also changed the .match to .test, we just need a bool there and .test is a bit faster than .match

if you are good with my patch, i would send you an pull request for that.

tykeal commented 10 years ago

Thanks for the link on the semicolons :)

That patch looks good.

tykeal commented 10 years ago

Closing with pull request #6