search

getmeli / meli

Platform for deploying static sites and frontend applications easily. Automatic SSL, deploy previews, reverse proxy, and more.
Other
2.41k stars 97 forks source link

feat: SAML login #158

Closed nikhiljha closed 3 years ago

nikhiljha commented 3 years ago

closes #8

gempain commented 3 years ago

@nikhiljha wow, this is great ! I'm looking forward to seeing this come to life, I really think this will be a great addition to Meli, thanks for taking the time to contribute, it's hugely appreciated 😀

nikhiljha commented 3 years ago

Thanks! A few notes on development that we should probably update in the README:

The last issue is that the OIDC passport strategy throws authentication requires session support. I'm a little bit stuck at this point since it doesn't look like meli uses express-session.

gempain commented 3 years ago
  • The default port is 8080 not 80

I'm looking at it and can't find where we reference 80. Are you referencing to the UI env.txt ?

  • You have to manually override MELI_API_URL when running outside of Docker, otherwise it checks for files in / which are obviously not there.

Are you referencing the UI ? MELI_API_URL is the environment variable set for telling the UI where Meli's server is. By default, in the UI, there's a file in public/env.txt which contains MELI_API_URL=http://localhost:8080 which should be the correct path for development, as this repo's docker-compose-dev.yml exposes Caddy on 8080.

I may be wrong, but I think you may be talking about MELI_UI_DIR which is an optional variable that we only set inside the unified Docker image to tell Caddy to serve the UI from /app/ui, but you shouldn't have to change this as development happens outside of Docker and MELI_UI_DIR is undefined by default.

The last issue is that the OIDC passport strategy throws authentication requires session support. I'm a little bit stuck at this point since it doesn't look like meli uses express-session.

It's true, we don't use passport-session as Meli is a stateless API basically, but if this is required for OIDC, I have no issue if you want to add it and configure it. I will review your code and configuration to make sure we don't miss anything but feel free to change what you want.

Before I dive into further research (which I will anyway), are you aware of any issues that could be created from introducing sessions ?

nikhiljha commented 3 years ago

I'm looking at it and can't find where we reference 80.

https://github.com/getmeli/meli/blob/82a1336859813661faf8a89e018346b7653c7a88/README.md#L153-L155

Are you referencing the UI?

Yes, forgot to mention that :P

there's a file in public/env.txt which contains MELI_API_URL=http://localhost:8080 which should be the correct path for development

This file wasn't correctly referenced for me. I had to edit the file where env.txt is loaded from and change it to ./env.txt instead.

are you aware of any issues that could be created from introducing sessions

None that I'm aware of, but that would be duplicating functionality (now there are tokens and sessions which both act as sessions).

gempain commented 3 years ago

Sorry for the delay, busy past few days !

https://github.com/getmeli/meli/blob/82a1336859813661faf8a89e018346b7653c7a88/README.md#L153-L155

It seems you have an outdated version, because I see this: https://github.com/getmeli/meli/blob/next/README.md

This file wasn't correctly referenced for me. I had to edit the file where env.txt is loaded from and change it to ./env.txt instead.

But this is really strange, because I have no problems locally. The code to load the env is here: https://github.com/getmeli/meli-ui/blob/next/src/providers/EnvProvider.tsx

And we load /env.txt, which is supposed to be inside /public in the project. Everything in that dir is placed at the root of React's build directory, and served under /, so I really don't see why you had to rename this file. I'm glad it worked, but I am clueless to what happened.

None that I'm aware of, but that would be duplicating functionality (now there are tokens and sessions which both act as sessions).

This is fine, if it's a requirement I can deal with it. I have time to test your PR today, so I'll get on this.

gempain commented 3 years ago

@nikhiljha I'm on it, testing locally. Note that we'll need to document this in the docs. I'm trying to setup OIDC locally with Hydra.

nikhiljha commented 3 years ago

I've been testing with Keycloak, and it looks like it authenticates successfully, but meli doesn't issue a token on callback (or, alternatively, check the session cookie instead of the token in the auth middleware) yet so it isn't functional.

I'll have time to work on this over the weekend, but if you already have a test setup working it shouldn't be too difficult to add the checking code :)

gempain commented 3 years ago

I don't have a working version. Dived deeper yesterday but could not authenticate. Don't you think it'd just be easier to implement the strategy manually ? I mean, it's just an HTTP call to the OIDC backend using the authorization code, and we only need the user ID, name, email and organizations, so that's a second HTTP call most likely, but other than that, I think it would be a lot easier and you could remove the session. What do you think ? For now I'll let you take a look when you have time, but once I get some free time next week I'll have a look. (BTW, working full time on Meli, worth mentioning).

nikhiljha commented 3 years ago

I looked at the OpenID Connect standard a little more closely and my brain is completely fried. I have no idea how to implement this without sessions.

So... this is now a SAML PR 😂.

If someone wants to implement OIDC later, the git history is still intact.

gempain commented 3 years ago

I do admit I haven't had enough time to fully understand it either, and I haven't succeeded in getting Hydra working with the OIDC plugin yet. I think SAML makes a lot of sense ! Let me know when you're done so I can have a look and we can merge 😄

nikhiljha commented 3 years ago

@gempain This should be good to review now. I made signing/verifying the SAML requests optional, but perhaps that should be mandatory to prevent misconfigurations.

gempain commented 3 years ago

@nikhiljha I review the PR. Good work ! I just have a few comments:

I've enabled Github Actions for pull requests here and for the UI. It should run when you push and help spot these issues.

nikhiljha commented 3 years ago

Oops, I made a change at the last second to rename "oidc" to "saml" and forgot about that.

@gempain If you want an easy way to test SAML, deploy Keycloak: https://www.keycloak.org/getting-started/getting-started-docker - setup takes maybe 15 minutes and you get OIDC and SAML :)

gempain commented 3 years ago

No worries, it can happen 😄

The build seems to break at the npm install phase. I searched for the error message sax not accessible from xml2js but I can't find anything online. I'm not sure what's going on there.

gempain commented 3 years ago

Moving to #218