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.

Sign up for GitHub

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

Add new group for permanent config #233

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Add new group for permanent config #233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7050232
Add new group for permanent config
PaulJKim Nov 2, 2023
c38380b
Reuse existing screens-admin group and use more descriptive access le…
PaulJKim Nov 9, 2023
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
2 changes: 1 addition & 1 deletion config/dev.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,5 @@ config :screenplay,

config :ueberauth, Ueberauth,
providers: [
cognito: {Screenplay.Ueberauth.Strategy.Fake, [groups: ["screenplay"]]}
cognito: {Screenplay.Ueberauth.Strategy.Fake, [groups: ["screenplay-emergency-admin"]]}
]
20 changes: 13 additions & 7 deletions lib/screenplay_web/auth_manager/auth_manager.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ defmodule ScreenplayWeb.AuthManager do

use Guardian, otp_app: :screenplay

@type access_level :: :none | :read_only | :admin
@type access_level :: :none | :read_only | :admin | :configurer

@screenplay_admin_group "screenplay"
@screenplay_admin_group "screenplay-emergency-admin"
@screenplay_configurer_group "screenplay-screen-configurer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhh! I just realized, we didn't need to add a new group but could just use the existing group called screens-admin. I'm not sure screenplay-screen-configurer is clearer than the name of the existing group. What do you think about nixing that new group and keeping the old one, especially since you're essentially renaming the group here to screenplay_configurer_group (or maybe screens_admin_group)

Copy link
Contributor Author

@PaulJKim PaulJKim Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When you refer to 'group description' are you just referring the name of the group (e.g. screenplay-emergency-admin) or something else? If it's just the name, I feel like screenplay-emergency-admin does a pretty decent job of describing a group that represents users who have access to the OFM takeover tool in Screenplay, but not sure if the 'description' of a group is a specific attribute that belongs to the cognito group?

And yeah that's a good point and agreed, we should just re-use the old screens-admin group rather than setting up a new one for Screenplay that will contain the same users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Group description. I agree the name screenplay-emergency-admin is better than screenplay, but the description of "Access to the Screenplay tool" is no longer quite accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that description. My gut tells me that changing the description wouldn't do anything to change what users belong to that group, but I don't know for sure. I can ask infra about that one to be sure though.


@spec subject_for_token(
resource :: Guardian.Token.resource(),
Expand All @@ -24,11 +25,16 @@ defmodule ScreenplayWeb.AuthManager do
def resource_from_claims(_), do: {:error, :invalid_claims}

@spec claims_access_level(Guardian.Token.claims()) :: access_level()
def claims_access_level(%{"groups" => groups}) do
if not is_nil(groups) and @screenplay_admin_group in groups do
:admin
else
:read_only
def claims_access_level(%{"groups" => groups}) when not is_nil(groups) do
cond do
@screenplay_admin_group in groups ->
:admin

@screenplay_configurer_group in groups ->
:configurer

true ->
:read_only
end
end

Expand Down
2 changes: 1 addition & 1 deletion test/support/conn_case.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ defmodule ScreenplayWeb.ConnCase do
Phoenix.ConnTest.build_conn()
|> Plug.Test.init_test_session(%{})
|> Guardian.Plug.sign_in(ScreenplayWeb.AuthManager, user, %{
"groups" => ["screenplay"]
"groups" => ["screenplay-emergency-admin"]
})
|> Plug.Conn.put_session(:username, user)

Expand Down
Loading