gabrielcsapo / node-git-server

🎡 A configurable git server written in Node.js
https://gabrielcsapo.github.io/node-git-server
MIT License
253 stars 73 forks source link

Authentication handler #29

Closed jlxip closed 6 years ago

jlxip commented 6 years ago

Useful if the server wants to evaluate the data (type of request and repository) and make a decision of whether the client has to authenticate or not.

echopoint commented 6 years ago

This is a duplicate authentication handler. This should just be a basic request / configuration handler and pass authentication off to the actual authentication handler authenticate.

jlxip commented 6 years ago

But if I modify the authenticate part it won't be backwards-compatible. Will it?

jlxip commented 6 years ago

Okay, have a look now. I've just cleaned the code. Which, by the way, is backwards-compatible.

gabrielcsapo commented 6 years ago

@jlxip I like this, but can we set defaults that will always call authenticate? The name authenticateHandler doesn't really read to the purpose, can we rename it?

gabrielcsapo commented 6 years ago

@echopoint thoughts on implementation? I feel like it could be much simpler, but I don't know what patterns can be used to make the experience less cumbersome.

gabrielcsapo commented 6 years ago

ohhh what if this was the interface

authenticate: function(type, repo, user, next) {
   if(type === 'clone' and repo === 'foo') {
    next();
  } else {
    user((username, password) => {
       next();
    });
  } 
}

The DX would be so much easier and the the person implementing would identify when the best case to get the username and password in real time.

jlxip commented 6 years ago

As far as I'm concerned, the current method seems... appropiate. This way, the developer can choose whether it's a completely private server or a partly-private one.

Edit: either way, it gives the user more ability to change stuff.

Edit 2: wait, I've actually thinked about @gabrielcsapo's idea and makes pretty much sense. That would be great and intuitive.

jlxip commented 6 years ago

Once said this, I'm closing the pull request, as that last code snippet is a way smarter solution to this issue. Thanks to everyone.

gabrielcsapo commented 6 years ago

@jlxip I opened another PR with the proposed idea here https://github.com/gabrielcsapo/node-git-server/pull/30. You made a really good point about how this was inflexible. I don't think my solution was smarter, you are the one who brought this idea up, it is all of our ideas :)