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

I would like to be able to set permissions from a DB table rather than hard coded in a permissions set. #709

Open
kairos0ne opened this issue Sep 19, 2022 · 8 comments

Comments

@kairos0ne
Copy link

Problem Statement

I tried to get the permissions structured correctly for Guardian in my Guardian module like so.

defmodule Dynamic.Guardian do

  alias Dynamic.BaseContent
  alias Dynamic.BaseStructures
  alias Dynamic.Repo


  use Guardian, otp_app: :dynamic,
    permissions: BaseStructures.get_table_permissions!()

However I get this error:

(RuntimeError) could not lookup Ecto repo Dynamic.Repo because it was not started or it does not exist

Stacktrace:
  │ (ecto 3.8.4) lib/ecto/repo/registry.ex:22: Ecto.Repo.Registry.lookup/1
  │ (ecto 3.8.4) lib/ecto/repo/supervisor.ex:159: Ecto.Repo.Supervisor.tuplet/2
  │ (dynamic 0.1.0) lib/dynamic/repo.ex:2: Dynamic.Repo.all/2
  │ (dynamic 0.1.0) lib/dynamic/base_structures.ex:145: Dynamic.BaseStructures.get_table_permissions!/0
  │ lib/dynamic/guardian.ex:9: (module)Elixir

Solution Brainstorm

Any thoughts on how to support this or if Im in fact missing something fundamental.

@yordis
Copy link
Member

yordis commented Nov 24, 2022

You should not do a function call at compile time to find the permissions (Notice that you are calling a function).

That is why when you try to compile the code, it fails because you haven't started the repository and whatnot needed to call the database.

Your intent could be achieved by passing an MFA configuration that could be called to fetch the permissions instead, something like:

  use Guardian, otp_app: :dynamic,
    permissions: {BaseStructures, :get_table_permissions!, []}

I am not sure if this is supported yet, but a PR is welcomed.

@geofflane
Copy link
Contributor

geofflane commented Nov 25, 2022

  use Guardian, otp_app: :dynamic,
    permissions: [BaseStructures, :get_table_permissions!, [])

I believe it's a tuple (threeple) {BaseStructures, :get_table_permissions!, []} see Guardian.Config.

But that won't work still because permissions are cached at compile time. (I just made it so only permissions are resolved at compile time instead of all config). That would need to be changed for this kind of resolution to work for permissions.

From what I'm seeing, the compile time pieces look like they might be optimizations that could just be read at runtime?

@yordis
Copy link
Member

yordis commented Nov 25, 2022

I believe it's a tuple (threeple) {BaseStructures, :get_table_permissions!, []} see Guardian.Config.

I fixed it; I meant to use a tuple instead of a list.

But that won't work still because permissions are cached at compile time.

I hear you. The idea is moving it at runtime 🤷🏻 otherwise, no way we could figure this one out as far as I can tell.

@kairos0ne
Copy link
Author

I forked this repo as a POC to see if I could approach it with this capability, fork instead of PR at first because I wasn't sure if this was done intentionally or not. Perhaps for added security ??? not sure.

POC here: https://github.com/kairos0ne/guardian

Its a bit rudimentary - Added a update_permissions(perms) function. It can certainly can be improved.

      @spec update_permissions(permissions :: map) :: :ok
      def update_permissions(permissions) do
        Application.put_env(unquote(otp_app), __MODULE__, Keyword.put(config(), :permissions, permissions))
      end

Then you can call update_permissions from you Guardian module implementation.

I also changed the permissions module to support run time updates.

If you guys have any suggestions or improvements let me know.

@yordis
Copy link
Member

yordis commented Nov 25, 2022

I forked this repo as a POC to see if I could approach it with this capability, fork instead of PR at first because I wasn't sure if this was done intentionally or not. Perhaps for added security ??? not sure.

Please create a PR in Draft. That will help me to follow exactly what you have done so far since otherwise it is hard for me to follow the work.

@kairos0ne
Copy link
Author

Sure I'll create a draft PR and a pull in all the changes np

@yordis
Copy link
Member

yordis commented Sep 12, 2023

@kairos0ne, any updates from your end?

@kairos0ne
Copy link
Author

Let me take a look...

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

No branches or pull requests

3 participants