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

Auth Application Workflow? [Docs] #153

Open
SimonLab opened this issue Oct 18, 2021 · 4 comments
Open

Auth Application Workflow? [Docs] #153

SimonLab opened this issue Oct 18, 2021 · 4 comments
Assignees
Labels
discuss Share your constructive thoughts on how to make progress with this issue T4h Time Estimate 4 Hours

Comments

@SimonLab
Copy link
Member

It took me a while to understand the workflow of the app after not working on it for a while. This issue contains my notes while I'm going over the code again. I'll convert them to documentation in the Readme and hopefully this will also help with #149

The application let you authenticate with Google, Github or by email:
image
This is done with the first part of the index controller which match the / endpoint:

case get_client_id_from_query(conn) do
# no auth_client_id means the request is for auth app
0 ->
Auth.Log.info(conn, params)
render_login_buttons(conn, params)

the second part of the index is used to authenticate user for another application. The user application redirect to the auth app and contains the auth_client_id query parameter, eg: /?auth_client_id=123

client_id ->
if client_id_valid?(client_id, conn) do
msg = "request with client_id: #{client_id} (index:73)"
Auth.Log.info(conn, Map.merge(params, %{msg: msg}))
render_login_buttons(conn, params)
else
msg = "auth_client_id: #{client_id} is not valid (index:77)"
Auth.Log.error(conn, Map.merge(params, %{msg: msg}))
conn
|> put_flash(:error, msg)
|> unauthorized(msg)
end

The redirection to the auth app is done using the auth_plug library:
https://github.com/dwyl/auth_plug/blob/77963c86483c78acb3f2fe386416d67b528607e8/lib/auth_plug.ex#L32-L39

    case AuthPlug.Token.verify_jwt(jwt) do
      {:ok, values} ->
        AuthPlug.Token.put_current_token(conn, jwt, values)

      # log the JWT verify error then redirect:
      {:error, reason} ->
        Logger.error("AuthPlug: " <> Kernel.inspect(reason))
        redirect_to_auth(conn, options) # redirect to auth application
    end

We can see that a jwt is validated and if it fails the user application redirect to the auth app with the auth_client_id:

    to =
      opts.auth_url <>
        "?referer=" <>
        URI.encode(baseurl <> conn.request_path) <>
        "&auth_client_id=" <> AuthPlug.Token.client_id()

see https://github.com/dwyl/auth_plug/blob/77963c86483c78acb3f2fe386416d67b528607e8/lib/auth_plug.ex#L47-L51

@SimonLab SimonLab added T4h Time Estimate 4 Hours discuss Share your constructive thoughts on how to make progress with this issue labels Oct 18, 2021
@SimonLab SimonLab self-assigned this Oct 18, 2021
SimonLab added a commit that referenced this issue Oct 18, 2021
@nelsonic
Copy link
Member

Hi @SimonLab thanks for opening this issue. 👍
Totally agree that we can do a much better job of explaining how the Auth App works.
The diagrams I've included in the REAMDE.md are very "high level"... we could probably use flow diagram. 💭
Very happy for someone with a fresh perspective to try.

To be clear: I don't expect that many people to deploy the Auth App themselves
because most people building Phoenix Apps will use mix gen.auth ...

Most people don't think in terms of code reuse or separation of concerns so they are fine with including a bunch of auth-related boilerplate in their main application; I very much prefer to separate things both for security and maintainability.
My objective has always been that people contributing to our "main" App could run it on their localhost with just one environment variable. I would prefer it to be zero environment variables to run the App. But I haven't figured out how to do that yet. Ideas/suggestions very much welcome!

Our priority is using the Auth App ourselves (via auth_plug) to authenticate in other Phoenix Apps.
e.g: dwyl/phoenix-chat-example#54
Anything we can do to streamline that process is good.

If other people decide to use e.g: #149 that's great! 🎉 We don't "lose" anything from them using it. There's every chance they will report back any issues they face which will help us to improve. However, my/our focus is not on building an Auth App. I see the Auth App is a byproduct of our attempt to simplify things in our main App.

@nelsonic
Copy link
Member

I find that reviewing the ERD of an App is immensely insightful when trying to figure out what is going on.
This is the current one for the auth app:

image

Generated manually using DBeaver [so that I can organise the tables logically ...] dwyl/learn-postgresql#84

@nelsonic nelsonic changed the title Application workflow Auth Application Workflow? [Docs] Oct 17, 2022
@SimonLab
Copy link
Member Author

Same feeling:

It took me a while to understand the workflow of the app after not working on it for a while

see #153 (comment)

So I'm going of the existing issues to list the features and requirements of the auth app, and to recap the why/what/how questions to see if I have more ideas to simplify the app process.

@nelsonic
Copy link
Member

Final snapshot of diagram:
image

Without much knowledge of database schemas,
if you just pay close attention,
you'll see that that the person_id relationship (foreign key)
is not correctly defined on 3 tables: apikeys, apps and roles.

auth-erd-person_id-fk-incomplete

This has caused me/us no end of pain while trying to add new features ... #231 (comment)

So, in addition to dramatically simplifying the database schema,
I will make sure that all the Ecto relationships are well-defined
so that we don't run into annoying constraint errors in the future.

If we start by deleting the tables that we don't need, we immediately simplify the ERD:
image

If we manually edit the diagram to include the person_id links (foreign keys):

auth-erd-tables-removed-person_id-edited

It becomes clearer what data "belongs" to a person.
But we can immediately spot something that incomplete/incorrect:
who does a group "belong" to? 🤷‍♂️

Obviously it's "unfair" to pick holes in a feature that is incomplete.
But it's "broken" and we (I) need to learn from it. 💭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Share your constructive thoughts on how to make progress with this issue T4h Time Estimate 4 Hours
Projects
None yet
Development

No branches or pull requests

2 participants