search

movableink / doorman

HTTP Proxy + OAuth
MIT License
167 stars 52 forks source link

Passport auth #54

Closed aackerman closed 4 years ago

aackerman commented 4 years ago

The main goal here is to replace everyauth, because it is using a soon to be unsupported auth mechanism for github. It's also been several years since everyauth has had a commit. Passport is getting ongoing support and feels like a good option for replacement.

I've also gone ahead and replaced jade with pug, which was very easy. I did this because npm auditing said that a dependency of the version of jade we use had an RCE advisory open.

I put all of the authentication logic in the ProxyDomain class that I created to rewrite the Domain class, this was mainly for ease of testing having everything in one file. I don't have an issue with moving it, but I think the separation would really only matter if we intended to add more auth schemes.

I did manual testing with all three auth schemes, including checking required email, domain, and github org, and handling for public paths.

Looking forward to getting some feedback on the changes.

mnutt commented 4 years ago

This looks great and came together amazingly fast. It'll take me a little bit to work through the full set of changes.

mchesler commented 4 years ago

I just noticed that this is branched from deploy-production. While deploy-production theoretically represents the right code to branch from, this should be a PR to merge into the multi-domain branch (which should probably be merged to master already)

aackerman commented 4 years ago

@mchesler I've updated the base branch.

aackerman commented 4 years ago

Ok, removed everyauth. I believe that's the last thing to do.