Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Implement OIDC #3170

Closed
wants to merge 1 commit into from
Closed

Conversation

lorenz
Copy link
Contributor

@lorenz lorenz commented Jul 21, 2023

Changes

This is an initial implementation of OIDC. It's still pretty basic, but it does log in users via OIDC. Automated user creation is not yet implemented, as is automatically assigning sites based on roles.

Mostly posted to gather some early feedback and for others to try.

Fixes #1554

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@bundlemon
Copy link

bundlemon bot commented Jul 21, 2023

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
492.34KB -
static/js/dashboard.js
318.55KB -
static/js/app.js
40.1KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.08KB -
tracker/js/plausible.js
742B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@jannismilz
Copy link

Still working on this?

@lorenz
Copy link
Contributor Author

lorenz commented Oct 15, 2023

@jannismilz I am currently using this as-is, as I said in the PR I wanted to get some feedback from the developers on the general approach before investing more time into it. What features do you need?

@jannismilz
Copy link

@lorenz Tbh I don't have specific needs I was just looking for a SSO login option. The method doesn't matter too much since I can use a IdP to handle that right?

@relu91
Copy link

relu91 commented Oct 27, 2023

I might be interested in this. I'm evaluating Plausible and having OIDC would be one selling point. I have two questions:

  • Can you resolve conflicts with the main branch? It would be easier to build and try.
  • As far as I understood, with OIDC_IDP configured properly, I still need to manually create users in Plausible but then my co-workers can simply log in with our SSO page. Is that correct? Since I manually create the users and I want to give a feeling that every is handled by our OIDC provider, would you recommend removing Plausible email verification?

@lorenz
Copy link
Contributor Author

lorenz commented Oct 27, 2023

I might be interested in this. I'm evaluating Plausible and having OIDC would be one selling point. I have two questions:

  • Can you resolve conflicts with the main branch? It would be easier to build and try.

I'l look into it today or sunday.

  • As far as I understood, with OIDC_IDP configured properly, I still need to manually create users in Plausible but then my co-workers can simply log in with our SSO page. Is that correct? Since I manually create the users and I want to give a feeling that every is handled by our OIDC provider, would you recommend removing Plausible email verification?

Currently yes, but it should be relatively easy to automatically create users from OIDC userinfo. Basically instead of bailing when the user is not found, it needs to call Plausible.Auth.User.new/Repo.insert.

@relu91
Copy link

relu91 commented Nov 20, 2023

Tried a image built in this branch but after initialization, it crashed:

warning: :logger :level has been set to :warn in config files, please use :warning instead
  (logger 1.15.7) lib/logger/app.ex:102: Logger.App.default_level/0
  (logger 1.15.7) lib/logger/app.ex:35: Logger.App.start/2
  (kernel 9.1) application_master.erl:293: :application_master.start_it_old/4
Kernel pid terminated (application_controller) ("{application_start_failure,plausible,{{shutdown,{failed_to_start_child,'Elixir.OpenIDConnect.Worker',{#{value => nil,protocol => 'Elixir.Enumerable',description => <<>>,'__struct__' => 'Elixir.Protocol.UndefinedError','__exception__' => true},[{'Elixir.Enumerable','impl_for!',1,[{file,\"lib/enum.ex\"},{line,1}]},{'Elixir.Enumerable',reduce,3,[{file,\"lib/enum.ex\"},{line,166}]},{'Elixir.Enum',map,2,[{file,\"lib/enum.ex\"},{line,4387}]},{'Elixir.Enum',into,3,[{file,\"lib/enum.ex\"},{line,1587}]},{'Elixir.OpenIDConnect.Worker',init,1,[{file,\"lib/openid_connect/worker.ex\"},{line,22}]},{gen_server,init_it,2,[{file,\"gen_server.erl\"},{line,962}]},{gen_server,init_it,6,[{file,\"gen_server.erl\"},{line,917}]},{proc_lib,init_p_do_apply,3,[{file,\"proc_lib.erl\"},{line,241}]}]}}},{'Elixir.Plausible.Application',start,[normal,[]]}}}")
Crash dump is being written to: erl_crash.dump...

Sadly, I can't easily get erl_crash.dump, I hope that that log is enough to understand the problem.

@lorenz
Copy link
Contributor Author

lorenz commented Nov 20, 2023

Is this with an OIDC provider set up? I think I see why it crashes if you don't. I'll get to fixing that at some point.

@ruslandoga
Copy link
Contributor

ruslandoga commented Nov 21, 2023

@lorenz @relu91 👋

The error means that the worker is started with provider_configs = nil, and since they are fetched from :openid_connect_providers app env on startup, I'd guess use_oidc = oidc_idp != "" evaluates to false in runtime.exs which means that OIDC_IDP env var was not set.

@relu91
Copy link

relu91 commented Nov 21, 2023

Ops my bad, I assumed the configuration was optional and I could add it later. Ok now everything starts but when I try to log in I get some good requests back and forth with our idp and then a 500 error from plausible. Here's the exeception raised in the log:

2023-11-21T11:57:53.500174999Z Request: GET /oidc/callback?code=<Code here>
2023-11-21T11:57:53.500179897Z ** (exit) an exception was raised:
2023-11-21T11:57:53.500183881Z     ** (ArgumentError) comparison with nil is forbidden as it is unsafe. If you want to check if a value is nil, use is_nil/1 instead
2023-11-21T11:57:53.500188081Z         (ecto 3.10.3) lib/ecto/query/builder.ex:1048: Ecto.Query.Builder.not_nil!/1
2023-11-21T11:57:53.500192285Z         (plausible 0.0.1) lib/plausible_web/controllers/auth_controller.ex:279: PlausibleWeb.AuthController.find_user/1
2023-11-21T11:57:53.500196544Z         (plausible 0.0.1) lib/plausible_web/controllers/auth_controller.ex:539: PlausibleWeb.AuthController.oidc_callback/2
2023-11-21T11:57:53.500216749Z         (plausible 0.0.1) lib/plausible_web/controllers/auth_controller.ex:1: PlausibleWeb.AuthController.action/2
2023-11-21T11:57:53.500221590Z         (plausible 0.0.1) lib/plausible_web/controllers/auth_controller.ex:1: PlausibleWeb.AuthController.phoenix_controller_pipeline/2
2023-11-21T11:57:53.500225910Z         (phoenix 1.7.7) lib/phoenix/router.ex:430: Phoenix.Router.__call__/5
2023-11-21T11:57:53.500229946Z         (plausible 0.0.1) lib/plausible_web/endpoint.ex:1: PlausibleWeb.Endpoint.plug_builder_call/2
2023-11-21T11:57:53.500234007Z         (plausible 0.0.1) lib/plausible_web/endpoint.ex:1: PlausibleWeb.Endpoint."call (overridable 3)"/2

Did I configure our IDP in the wrong way?

@ruslandoga
Copy link
Contributor

ruslandoga commented Nov 21, 2023

The error means that email is nil here. And that it shouldn't be.
It's nil probably because claims doesn't contain "email" field.

I don't know anything about OIDC but from just looking at the code, the IDP didn't respond with all the required scopes.
In particular, email is missing.

@relu91
Copy link

relu91 commented Nov 21, 2023

The error means that email is nil here. And that it shouldn't be. It's nil probably because claims doesn't contain "email" field.

I don't know anything about OIDC but from just looking at the code, the IDP didn't respond with all the required scopes. In particular, email is missing.

exactly the problem was that zitadel (our idp) didn't send the user details by the defult in the token. You have to manually enable it in the project dashboard. Works like a charm now! The only thing left for me is user creation. I'm struggling to find a sort of admin dashboard where I can manually create users. Is it there? meanwhile I'll try to navigate the docs.

@ruslandoga
Copy link
Contributor

I don't think there is an admin dashboard like that.

@relu91
Copy link

relu91 commented Nov 22, 2023

Update

I was able to get users to automatically register when using our IDP. As you might have guessed I've never coded in Elixir, so I had to kinda guess how it works (plus a little help from chatGPT - to be honest this time it got me sideway multiple times). For reference this is how the function coded by @lorenz looks now:

  def oidc_callback(conn, %{"code" => code}) do
    if !Application.get_env(:plausible, :use_oidc) do
      render_error(
          conn,
          400,
          "OIDC is not active"
        )
    else
      with {:ok, tokens} <- OpenIDConnect.fetch_tokens(:default, %{code: code}),
          {:ok, claims} <- OpenIDConnect.verify(:default, tokens["id_token"]),
          {{:ok, user}, claims} <- {find_user(claims["email"]), claims} do
            login_dest = get_session(conn, :login_dest) || Routes.site_path(conn, :index)

            conn
            |> put_session(:current_user_id, user.id)
            |> put_resp_cookie("logged_in", "true",
              http_only: false,
              max_age: 60 * 60 * 24
            )
            |> put_session(:login_dest, nil)
            |> redirect(to: login_dest)
      else
        result ->
          case result do
            {:user_not_found, claims} ->
              IO.inspect(claims)
              with {:ok, user} <- Plausible.Auth.create_user(claims["given_name"],claims["email"],"cP943<@Tti'B") do
                login_dest = get_session(conn, :login_dest) || Routes.site_path(conn, :index)
                conn
                  |> put_session(:current_user_id, user.id)
                  |> put_resp_cookie("logged_in", "true",
                    http_only: false,
                    max_age: 60 * 60 * 24
                  )
                  |> put_session(:login_dest, nil)
                  |> redirect(to: login_dest)
              else
                e -> render_error(
                  conn,
                  400,
                  "Error creating the user: #{inspect(e)}"
                )
              end
            {e,_} -> render_error(
              conn,
              400,
              "OIDC login failed: #{inspect(e)}"
            )
            e -> render_error(
              conn,
              400,
              "OIDC login failed: #{inspect(e)}"
            )
          end
      end

It is kinda ugly and it has duplicated code, but it works for our use case. I would say that before going to production what I would like to see is:

  • UI coherence: the landing page of plausible still contains the register link which does not really make sense when you have the IDP enabled.
  • Ability to restrict the users that can use plausible. This can achieved by looking for a specific role or something else?
  • Ability to disable Add website button to some users. Maybe this is already a feature but I couldn't find it in the doc.

@xavivars
Copy link

We're considering adopting Plausible as the analytics tool for a non-profit website (with 10+ millions visits monthly), but having SSO is a pretty strong requirement for us.

We use keycloak to manage user permissions across all the tools we use, and having this feature being considered would make it much easier for the board to accept Plausible as the tool of choice

@kloenk kloenk mentioned this pull request Jan 21, 2024
8 tasks
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ukutaht
Copy link
Contributor

ukutaht commented Apr 23, 2024

I already closed the other OIDC pull request, will do the same with this one. See #3706 (comment) for reasoning. In short: we consider team accounts as a blocker for SSO

@ukutaht ukutaht closed this Apr 23, 2024
@vai
Copy link

vai commented Jun 2, 2024

For everyone's clarity - does that mean that the features that enable Self-Hosted users to run CE and use their own IdP aren't being merged at all, or aren't being merged yet? Critically, I'd like to know I can self-host and use SSO with my own provider - even if I do have to make use of Traefik and headers to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Login with SAML/SSO
8 participants