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

Topt enrollment backend procedure #2651

Merged
merged 8 commits into from
May 27, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions lib/trento/users.ex
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,61 @@ defmodule Trento.Users do
end
end

def reset_totp(%User{id: 1}), do: {:error, :forbidden}

def reset_totp(%User{} = user) do
update_user_totp(user, %{
totp_enabled_at: nil,
totp_secret: nil,
totp_last_used_at: nil
})
end

def initiate_totp_enrollment(%User{totp_enabled_at: nil} = user) do
result =
Ecto.Multi.new()
|> Ecto.Multi.run(:user_without_totp, fn _, _ ->
CDimonaco marked this conversation as resolved.
Show resolved Hide resolved
reset_totp(user)
end)
|> Ecto.Multi.run(
:user_totp_enrolled,
fn _, %{user_without_totp: user} ->
update_user_totp(user, %{
totp_secret: NimbleTOTP.secret()
})
end
)
|> Repo.transaction()

case result do
{:ok, %{user_totp_enrolled: %User{totp_secret: totp_secret, username: username}}} ->
{:ok,
%{
secret: totp_secret,
secret_qr_encoded:
NimbleTOTP.otpauth_uri("trento:#{username}", totp_secret, issuer: "Trento")
}}

{:error, _, changeset_error, _} ->
{:error, changeset_error}
end
end

def initiate_totp_enrollment(_), do: {:error, :totp_already_enabled}

def confirm_totp_enrollment(%User{id: 1}, _), do: {:error, :forbidden}

def confirm_totp_enrollment(%User{totp_secret: totp_secret, totp_enabled_at: nil} = user, totp) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to have a more common naming, i named this totp , totp_code.
Which one do you prefer? To put the same name in the other validate_totp function in my PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better totp_code

if NimbleTOTP.valid?(totp_secret, totp) do
now = DateTime.utc_now()
update_user_totp(user, %{totp_enabled_at: now, totp_last_used_at: now})
else
{:error, :enrollment_totp_not_valid}
end
end

def confirm_totp_enrollment(_, _), do: {:error, :totp_already_enabled}

defp maybe_set_locked_at(%{enabled: false} = attrs) do
Map.put(attrs, :locked_at, DateTime.utc_now())
end
Expand Down Expand Up @@ -198,4 +253,12 @@ defmodule Trento.Users do
rescue
Ecto.StaleEntryError -> {:error, :stale_entry}
end

defp update_user_totp(%User{id: 1}, _), do: {:error, :forbidden}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did put this update_user_totp as public function, but it is true that I don't use it outside.
I was simply using in the iex console.
Thinking this way, once you have the enrollment, you don't really need it.
I might use your private version hehe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, private is better we reduce the api surface and it's already tested by the public functions


defp update_user_totp(%User{} = user, attrs) do
user
|> User.totp_update_changeset(attrs)
|> Repo.update()
end
end
14 changes: 14 additions & 0 deletions lib/trento_web/controllers/fallback_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,20 @@ defmodule TrentoWeb.FallbackController do
|> render(:"428")
end

def call(conn, {:error, :totp_already_enabled}) do
conn
|> put_status(:unprocessable_entity)
|> put_view(ErrorView)
|> render(:"422", reason: "TOTP already enabled, could not process the enrollment procedure")
end

def call(conn, {:error, :enrollment_totp_not_valid}) do
conn
|> put_status(:unprocessable_entity)
|> put_view(ErrorView)
|> render(:"422", reason: "TOTP code not valid for the enrollment procedure.")
end

def call(conn, {:error, [error | _]}), do: call(conn, {:error, error})

def call(conn, {:error, _}) do
Expand Down
57 changes: 57 additions & 0 deletions lib/trento_web/controllers/v1/profile_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,61 @@ defmodule TrentoWeb.V1.ProfileController do
render(conn, "profile.json", user: updated_user)
end
end

operation :reset_totp,
summary: "Reset the TOTP configuration for the user",
tags: ["Platform"],
responses: [
forbidden: Schema.Forbidden.response()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess no_content should be part of responses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

]

def reset_totp(conn, _) do
%User{} = user = Pow.Plug.current_user(conn)

with {:ok, %User{}} <- Users.reset_totp(user) do
send_resp(conn, :no_content, "")
end
end

operation :get_totp_enrollment_data,
summary: "Get TOTP enrollment data",
tags: ["Platform"],
responses: [
ok:
{"UserTOTPEnrollmentPayload", "application/json", Schema.User.UserTOTPEnrollmentPayload},
unprocessable_entity: Schema.UnprocessableEntity.response(),
forbidden: Schema.Forbidden.response()
]

def get_totp_enrollment_data(conn, _) do
%User{} = user = Pow.Plug.current_user(conn)

with {:ok, enrollment_payload} <- Users.initiate_totp_enrollment(user) do
render(conn, "totp_enrollment_data.json", enrollment_payload: enrollment_payload)
end
end

operation :confirm_totp_enrollment,
summary: "Confirm TOTP enrollment procedure",
tags: ["Platform"],
request_body:
{"UserTOTPEnrollmentConfirmRequest", "application/json",
Schema.User.UserTOTPEnrollmentConfirmRequest},
responses: [
ok:
{"TOTP Enrollment completed", "application/json",
Schema.User.UserTOTPEnrollmentConfirmPayload},
unprocessable_entity: Schema.UnprocessableEntity.response(),
forbidden: Schema.Forbidden.response()
]

def confirm_totp_enrollment(%{body_params: body_params} = conn, _) do
%User{} = user = Pow.Plug.current_user(conn)
totp = Map.get(body_params, :totp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'm using totp_code in the other PR. Let's use the same term.
I will adapt my PR once we have it decided

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totp_code also here and in the schemas!


with {:ok, %User{totp_enabled_at: totp_enabled_at}} <-
Users.confirm_totp_enrollment(user, totp) do
render(conn, "totp_enrollment_completed.json", %{totp_enabled_at: totp_enabled_at})
end
end
end
65 changes: 65 additions & 0 deletions lib/trento_web/openapi/v1/schema/user.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,71 @@ defmodule TrentoWeb.OpenApi.V1.Schema.User do

alias TrentoWeb.OpenApi.V1.Schema.Ability.AbilityCollection

defmodule UserTOTPEnrollmentPayload do
@moduledoc false

@schema %Schema{
title: "UserTOTPEnrollmentPayload",
description: "Trento User totp enrollment payload",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put topt as upper case TOTP as in other texts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

type: :object,
additionalProperties: false,
properties: %{
secret: %Schema{type: :string, description: "TOTP secret", nullable: false},
secret_qr_encoded: %Schema{
type: :string,
description: "TOTP secret qr encoded",
nullable: false
}
},
required: [:secret, :secret_qr_encoded]
}

def schema, do: @schema
end

defmodule UserTOTPEnrollmentConfirmPayload do
@moduledoc false

@schema %Schema{
title: "UserTOTPEnrollmentConfirmPayload",
description: "Trento User TOTP enrollment completed payload",
type: :object,
additionalProperties: false,
properties: %{
totp_enabled_at: %OpenApiSpex.Schema{
type: :string,
format: :"date-time",
description: "Date of TOTP enrollment",
nullable: false
}
},
required: [:totp_enabled_at]
}

def schema, do: @schema
end

defmodule UserTOTPEnrollmentConfirmRequest do
@moduledoc false

@schema %Schema{
title: "UserTOTPEnrollmentConfirmRequest",
description: "Trento User totp enrollment confirmation payload",
type: :object,
additionalProperties: false,
properties: %{
totp: %Schema{
type: :string,
description: "TOTP generated from enrollment secret",
nullable: false
}
},
required: [:totp]
}

def schema, do: @schema
end

defmodule UserProfile do
@moduledoc false

Expand Down
3 changes: 3 additions & 0 deletions lib/trento_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ defmodule TrentoWeb.Router do

get "/profile", ProfileController, :show
patch "/profile", ProfileController, :update
delete "/profile/totp_enrollment", ProfileController, :reset_totp
get "/profile/totp_enrollment", ProfileController, :get_totp_enrollment_data
post "/profile/totp_enrollment", ProfileController, :confirm_totp_enrollment

get "/abilities", AbilityController, :index

Expand Down
13 changes: 13 additions & 0 deletions lib/trento_web/views/v1/profile_view.ex
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,17 @@ defmodule TrentoWeb.V1.ProfileView do
updated_at: updated_at
}
end

def render("totp_enrollment_completed.json", %{
totp_enabled_at: totp_enabled_at
}),
do: %{totp_enabled_at: totp_enabled_at}

def render("totp_enrollment_data.json", %{
enrollment_payload: %{
secret: secret,
secret_qr_encoded: secret_qr_encoded
}
}),
do: %{secret: Base.encode32(secret, padding: false), secret_qr_encoded: secret_qr_encoded}
end
3 changes: 2 additions & 1 deletion mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ defmodule Trento.MixProject do
{:x509, "~> 0.8.8"},
{:argon2_elixir, "~> 4.0"},
{:ecto_commons, "~> 0.3.4"},
{:bodyguard, "~> 2.4"}
{:bodyguard, "~> 2.4"},
{:nimble_totp, "~> 1.0"}
]
end

Expand Down
1 change: 1 addition & 0 deletions mix.lock
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"mimerl": {:hex, :mimerl, "1.2.0", "67e2d3f571088d5cfd3e550c383094b47159f3eee8ffa08e64106cdf5e981be3", [:rebar3], [], "hexpm", "f278585650aa581986264638ebf698f8bb19df297f66ad91b18910dfc6e19323"},
"mox": {:hex, :mox, "1.1.0", "0f5e399649ce9ab7602f72e718305c0f9cdc351190f72844599545e4996af73c", [:mix], [], "hexpm", "d44474c50be02d5b72131070281a5d3895c0e7a95c780e90bc0cfe712f633a13"},
"nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"},
"nimble_totp": {:hex, :nimble_totp, "1.0.0", "79753bae6ce59fd7cacdb21501a1dbac249e53a51c4cd22b34fa8438ee067283", [:mix], [], "hexpm", "6ce5e4c068feecdb782e85b18237f86f66541523e6bad123e02ee1adbe48eda9"},
"open_api_spex": {:hex, :open_api_spex, "3.18.1", "0a73cd5dbcba7d32952dd9738c6819892933d9bae1642f04c9f200281524dd31", [:mix], [{:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:plug, "~> 1.7", [hex: :plug, repo: "hexpm", optional: false]}, {:poison, "~> 3.0 or ~> 4.0 or ~> 5.0", [hex: :poison, repo: "hexpm", optional: true]}, {:ymlr, "~> 2.0 or ~> 3.0 or ~> 4.0", [hex: :ymlr, repo: "hexpm", optional: true]}], "hexpm", "f52933cddecca675e42ead660379ae2d3853f57f5a35d201eaed85e2e81517d1"},
"parallel_stream": {:hex, :parallel_stream, "1.1.0", "f52f73eb344bc22de335992377413138405796e0d0ad99d995d9977ac29f1ca9", [:mix], [], "hexpm", "684fd19191aedfaf387bbabbeb8ff3c752f0220c8112eb907d797f4592d6e871"},
"parse_trans": {:hex, :parse_trans, "3.4.1", "6e6aa8167cb44cc8f39441d05193be6e6f4e7c2946cb2759f015f8c56b76e5ff", [:rebar3], [], "hexpm", "620a406ce75dada827b82e453c19cf06776be266f5a67cff34e1ef2cbb60e49a"},
Expand Down
5 changes: 4 additions & 1 deletion test/support/factory.ex
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,10 @@ defmodule Trento.Factory do
username: Faker.Pokemon.name(),
deleted_at: nil,
locked_at: nil,
password_change_requested_at: nil
password_change_requested_at: nil,
totp_enabled_at: nil,
totp_secret: nil,
totp_last_used_at: nil
}
end

Expand Down
2 changes: 1 addition & 1 deletion test/trento/discovery/policies/cluster_policy_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1184,7 +1184,7 @@ defmodule Trento.Discovery.Policies.ClusterPolicyTest do
|> ClusterPolicy.handle(nil)
end

test "should set the health to critical when the SAPInstance resourece is running the same node" do
test "should set the health to critical when the SAPInstance resource is running the same node" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess @CDimonaco found the same issue a second time here @janvhs 👍

https://github.com/trento-project/web/pull/2650/files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 great ideas are in the air

group_1_resources =
build_list(1, :crm_resource, %{
"Id" => "rsc_sap_NWP_ASCS00",
Expand Down
89 changes: 89 additions & 0 deletions test/trento/users_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -435,5 +435,94 @@ defmodule Trento.UsersTest do
refute deleted_at == nil
assert [] == Trento.Repo.all(from u in UsersAbilities, where: u.user_id == ^user_id)
end

test "reset_totp/1 reset user topt values" do
user =
insert(:user, %{
totp_enabled_at: DateTime.utc_now(),
totp_secret: Faker.Internet.domain_name(),
totp_last_used_at: DateTime.utc_now()
})

assert {:ok,
%User{
totp_enabled_at: nil,
totp_secret: nil,
totp_last_used_at: nil
}} = Users.reset_totp(user)
end

test "initiate_totp_enrollment/1 returns error if the totp is already configured for the user" do
user =
insert(:user, %{
totp_enabled_at: DateTime.utc_now(),
totp_secret: Faker.Internet.domain_name(),
totp_last_used_at: DateTime.utc_now()
})

assert {:error, :totp_already_enabled} == Users.initiate_totp_enrollment(user)
end

test "initiate_totp_enrollment/1 could not initiate enrollment for the default admin" do
assert {:error, :forbidden} == Users.initiate_totp_enrollment(%User{id: 1})
end

test "initiate_totp_enrollment/1 returns a totp secret for enrollment" do
user =
insert(:user)

{:ok, %{secret: secret, secret_qr_encoded: secret_qr_encoded}} =
Users.initiate_totp_enrollment(user)

assert {:ok, %User{totp_secret: totp_secret, totp_enabled_at: nil, totp_last_used_at: nil}} =
Users.get_user(user.id)

assert secret == totp_secret
refute secret_qr_encoded == nil
end

test "confirm_totp_enrollment/2 returns error if the user has already the totp enabled" do
user =
insert(:user, %{
totp_enabled_at: DateTime.utc_now(),
totp_secret: Faker.Internet.domain_name(),
totp_last_used_at: DateTime.utc_now()
})

assert {:error, :totp_already_enabled} == Users.confirm_totp_enrollment(user, "123")
end

test "confirm_totp_enrollment/2 returns error if the user is the default admin" do
assert {:error, :forbidden} == Users.confirm_totp_enrollment(%User{id: 1}, "123")
end

test "confirm_totp_enrollment/2 returns error if the totp is not valid for the secret" do
user =
insert(:user, %{
totp_enabled_at: nil,
totp_secret: Faker.Internet.domain_name(),
totp_last_used_at: nil
})

assert {:error, :enrollment_totp_not_valid} == Users.confirm_totp_enrollment(user, "123")
end

test "confirm_totp_enrollment/2 returns the updated user with otp configured if the otp is valid for enrollment secret" do
secret = NimbleTOTP.secret()

user =
insert(:user, %{
totp_enabled_at: nil,
totp_secret: secret,
totp_last_used_at: nil
})

assert {:ok, %User{totp_enabled_at: totp_enabled_at, totp_last_used_at: totp_last_used_at}} =
Users.confirm_totp_enrollment(user, NimbleTOTP.verification_code(secret))

refute totp_enabled_at == nil
refute totp_last_used_at == nil
assert totp_enabled_at == totp_last_used_at
end
end
end
Loading
Loading