Add (REST) API for managing (GraphQL) API credentials(/tokens) #436

johnshaughnessy · 2020-12-09T01:49:24Z

No description provided.

johnshaughnessy · 2020-12-10T04:12:48Z

lib/ret/api/token_utils.ex

+  defp ensure_atom(x) when is_binary(x), do: String.to_atom(x)
+
+  # TODO Should we permit the creation of tokens
+  # on behalf of other accounts like this?


It feels odd to allow this, but it seemed like an easy way for us to do a closed alpha/beta test of the API on HMC. Alternatively, we could add a column (e.g. an integer bitmask) to the database and use it to assign roles, then check these roles to see if the given account has permission to create credentials.

johnshaughnessy · 2020-12-10T04:15:08Z

lib/ret/hub.ex

+        scopes: scopes,
+        account_id: account_id
+      }) do
+    # TODO: Is it correct to disallow the create_accounts scope for account type tokens?


I got it into my head that only :app credentials would be allowed to have the create_accounts scope, but then I don't have a good reason for this. Maybe only admin accounts should be allowed to create :account credentials that have the create_accounts scope. Then the only important distinction between :app credentials and :account credentials is the fact that :app credentials are accessible / managed / shared by all admin accounts.

I was a bit confused by this since I thought we removed create_accounts from #399 since it was not implemented. But I see create_accounts still exists in Ret.Api.Scaopes. I would suggest we remove create_accounts from these PRs altogether and also, importantly, ensure that input validation rejects claims that don't exist before it even gets to this point.

johnshaughnessy · 2020-12-10T04:15:55Z

lib/ret/hub.ex

+  end
+
+  def can?(%Ret.Account{}, :list_credentials, :account) do
+    # TODO: Allow admins to disable this in config


I don't know how/where to add config values. I'll have to learn

Happy to help if needed. See examples of where we use "features|public_rooms" and get_config_bool for reference. We might want to introduce a cached version, get_cached_config_bool if we're concerned about database hits for config values, and if a stale cache delay is acceptable.

Added in f52a5c2 and Hubs-Foundation/hubs#3551

johnshaughnessy · 2020-12-10T04:16:33Z

lib/ret_web/controllers/api/v2/credentials_controller.ex

+    only: [to_claims: 2, authed_create_credentials: 2, authed_list_credentials: 2, authed_revoke_credentials: 2]
+
+  # Limit to 1 TPS
+  # TODO : I copied this from other controllers. Is this an appropriate limit? -John


Perhaps the same limit should be added to the graphql endpoint

Definitely seems appropriate for this API. Not sure about the entire graphql endpoint.

lib/ret_web/controllers/api/v2/credentials_controller.ex

brianpeiris

Looks good, though we should definitely resolve our approach to this in general, as we discussed.

brianpeiris · 2020-12-10T23:22:50Z

lib/ret/api/credentials.ex

@@ -87,11 +87,11 @@ defmodule Ret.Api.Credentials do
    end
  end

-  defp validate_subject_type(:subject_type, subject_type) do
+  def validate_subject_type(:subject_type, subject_type) do


nit: Might be better to abstract these a bit, instead of exposing the callback used by validate_change. It's a bit awkward to have the callers repeat :scopes and :subject_type. In fact, it may be better to create a private validate_field function that overloads for each field type instead, and use that for validate_change. Something like:

defp validate_field(:scopes, scopes) do ... end defp validate_field(:subject_type, scopes) do ... end def validate_scopes(scopes) do validate_field(:scopes, scopes) end def validate_subject_type(subject_type) do validate_field(:subject_type, subject_type) end

end

brianpeiris · 2020-12-11T00:14:45Z

lib/ret/api/token_utils.ex

+  defp validate_account_id(%Account{}, account_id) do
+    case Account.query()
+         |> Account.where_account_id_is(account_id)
+         |> Repo.one() do


I'd suggest using Repo.exists? instead of actually getting the account object.

brianpeiris · 2020-12-11T16:13:00Z

lib/ret/api/token_utils.ex

+    with [] <-
+           validate_account_id(account, account_id) ++
+             Credentials.validate_scopes_type(:scopes, scopes) ++
+             Credentials.validate_subject_type(:subject_type, subject_type) do


nit: Not sure if we've been using this pattern already, but it does feel a bit too clever to use with in this way, since we're not actually doing anything with the matched value and implicitly allowing with to return the error list. Seems like this is one of those times where it's better to be clear than concise.

brianpeiris · 2020-12-11T17:28:33Z

lib/ret/hub.ex

+        scopes: scopes,
+        account_id: account_id
+      }) do
+    # TODO: Is it correct to disallow the create_accounts scope for account type tokens?


I was a bit confused by this since I thought we removed create_accounts from #399 since it was not implemented. But I see create_accounts still exists in Ret.Api.Scaopes. I would suggest we remove create_accounts from these PRs altogether and also, importantly, ensure that input validation rejects claims that don't exist before it even gets to this point.

brianpeiris · 2020-12-11T17:36:02Z

lib/ret/hub.ex

+  end
+
+  def can?(%Ret.Account{}, :list_credentials, :account) do
+    # TODO: Allow admins to disable this in config


Happy to help if needed. See examples of where we use "features|public_rooms" and get_config_bool for reference. We might want to introduce a cached version, get_cached_config_bool if we're concerned about database hits for config values, and if a stale cache delay is acceptable.

brianpeiris · 2020-12-11T17:44:44Z

lib/ret_web/controllers/api/v2/credentials_controller.ex

+    case Credentials.query()
+         |> Credentials.where_sid_is(credentials_sid)
+         |> Repo.one() do


I'd be inclined to use Repo.get_by() instead of constructing a query.

brianpeiris · 2020-12-11T18:09:32Z

lib/ret_web/views/api/v2/credentials_view.ex

+  end
+
+  def render("show.json", %{credentials: credentials}) do
+    render("show.json", %{credentials: credentials, token: nil})


Bit strange that token is on the outer result here, but inside credentials: in the other views.

I may be misunderstanding your comment - but token always ends up as a key inside the credentials json, not outside it. This line is just redirecting to the other render("show.json", ...) (which does the call to render_credentials and merges in the token).

Open to suggestions to make this clearer -- My intention is to return a token in the credentials object when you first create one, but then have token: null every time after that.

Ah, my mistake, I misread the code paths in this case.

brianpeiris · 2020-12-11T18:19:14Z

lib/ret/hub.ex

+    # TODO: Is it correct to disallow the create_accounts scope for account type tokens?
+    # If so, why? Do we need to enforce this everywhere?
+    Scopes.create_accounts() not in scopes
+  end


Maybe we should only allow admins to create credentials for the first release, at least until we decide on doing roles?

brianpeiris · 2020-12-11T18:22:27Z

test/ret_web/controllers/api/credentials_controller_test.exs

+           |> get(list)
+           |> json_response(200)
+           |> Map.get("credentials")
+           |> Enum.count() === 0


We do this enough in the tests that it should be a get_credentials_count helper.

brianpeiris · 2020-12-11T18:27:19Z

test/ret_web/controllers/api/credentials_controller_test.exs

+           |> hd()
+           |> Map.get("is_revoked")
+  end
+end


👍 nice tests!

Thanks :)

I have not wrapped my head around or studied up on best practices for testing elixir + phoenix apps. Here I test an interface (CredentialsController) and the model (Credentials) together, which I could have done this separately. I am happy enough with it as it is, but want to be better about this in the future. This seems like it'll be increasingly important as we have alternative interfaces, transports, and views (REST over HTTP, graphql, websockets passing json).

Allow admin accounts to create creds on behalf of other accounts

johnshaughnessy force-pushed the feature/credentials-api branch from 5b7450d to 4e54d7e Compare December 10, 2020 03:44

johnshaughnessy changed the title ~~Add REST API for managing graphql API tokens/credentials~~ Add (REST) API for managing (GraphQL) API credentials(/tokens) Dec 10, 2020

johnshaughnessy marked this pull request as ready for review December 10, 2020 03:53

johnshaughnessy commented Dec 10, 2020

View reviewed changes

lib/ret_web/controllers/api/v2/credentials_controller.ex Show resolved Hide resolved

brianpeiris reviewed Dec 11, 2020

View reviewed changes

johnshaughnessy mentioned this pull request Dec 14, 2020

Add feature flag for public API access Hubs-Foundation/hubs#3551

Merged

johnshaughnessy added 7 commits December 14, 2020 11:34

Add (regular) API to manage (graphql) credentials

a02026d

Expand admin account permissions

2b633c1

Allow admin accounts to create creds on behalf of other accounts

Remove create_accounts scope

e448b99

Create test helper

80c275d

PR Feedback

3b49320

Require a server-level flag for graphql api

ca9ab00

lint

d3adae7

johnshaughnessy force-pushed the feature/credentials-api branch from 1ea3272 to d3adae7 Compare December 14, 2020 19:36

johnshaughnessy merged commit 2de8003 into feature/graphql-2 Dec 14, 2020

johnshaughnessy deleted the feature/credentials-api branch December 14, 2020 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add (REST) API for managing (GraphQL) API credentials(/tokens) #436

Add (REST) API for managing (GraphQL) API credentials(/tokens) #436

johnshaughnessy commented Dec 9, 2020

johnshaughnessy Dec 10, 2020

johnshaughnessy Dec 10, 2020

brianpeiris Dec 11, 2020

johnshaughnessy Dec 10, 2020

brianpeiris Dec 11, 2020

johnshaughnessy Dec 14, 2020

johnshaughnessy Dec 10, 2020

brianpeiris Dec 11, 2020

brianpeiris left a comment

brianpeiris Dec 10, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

johnshaughnessy Dec 14, 2020

brianpeiris Dec 15, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

brianpeiris Dec 11, 2020

johnshaughnessy Dec 14, 2020

Add (REST) API for managing (GraphQL) API credentials(/tokens) #436

Add (REST) API for managing (GraphQL) API credentials(/tokens) #436

Conversation

johnshaughnessy commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianpeiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment