resource/app owner schema flexibility

[techgaun]

right now, the owner schema is assumed to have an integer type and id as the default field(as per ecto belongs_to/3) and there are at least two parts where I can see we need the modification for the very least:

• migration script (need to be able to specify integer vs uuid or even anything else for example)
• belongs_to stuff where we might need to pass additional opts using Ecto.Schema.belongs_to/3 so we can possibly specify custom state of our foreign key

One of the quick ideas I had was to add resource_owner_opts and application_owner_opts which I've in master...techgaun:owner-config and am using it right now for my purpose. It does not take care of migration of course but it was simple enough for me to modify generated migration script.

This works fine so far for my case (my resource owner table has uuid type with user_id as the field name). I have not looked yet at most of the code to have more clear idea but what do you recommend in this case? My solution might be fine as long as we advise users to update generated migration scripts to adjust with additional options passed to belongs_to in schema but longer term, this may feel like a hack. Thoughts?

It would be great if we can generalize for cases like this so we don't tie the resource owner to be of particular schema.

Thoughts?

[danschultzer]

Thanks for the work @techgaun! I think the way you've set it up is the most obvious fix, but reading the documentation we could have a cleaner solution by using @foreign_key_type macro:

# Define a module to be used as base
defmodule MyApp.Schema do
  defmacro __using__(_) do
    quote do
      use Ecto.Schema
      @primary_key {:id, :binary_id, autogenerate: true}
      @foreign_key_type :binary_id
    end
  end
end

# Now use MyApp.Schema to define new schemas
defmodule MyApp.Comment do
  use MyApp.Schema

  schema "comments" do
    belongs_to :post, MyApp.Post
  end
end

So we could permit an :app_schema config field to be used, and the only configuration necessary would be this:

config :ex_oauth2_provider, ExOauth2Provider,
  # ...
  app_schema: MyApp.Schema

As for the migration file, I think that will be the developer's responsibility to change after running mix ex_oauth2_provider.install. This library should just have a section in the readme on UUID setup. Ecto does it this way too, making the assumption that by default an autoincrementing field is used as the primary field.

Also, could you show me how you have the resource owner schema currently set up with UUID? Wouldn't all primary id's in your project have to be UUID? I haven't worked with UUID and Ecto before.

[danschultzer]

I've created #21 but haven't tested it myself yet with UUID. Does this work for you @techgaun?

[techgaun]

@danschultzer your solution is probably a good idea longer term. My setup is in fact almost same as what you are proposing for the system I am working on because we have certain schemas that have integer field as the primary keys and some with uuids as primary keys as per our need.

I personally like being explicit on things like schemas so it is instantly obvious just after install of this package so maybe we could even let users change each of these schemas generated by this library.

config :ex_oauth2_provider, ExOauth2Provider,
  oauth_app_schema: MyApp.OauthApplications,
  # so on for tokens, grants, etc.

As for this particular issue itself, I think the PR you made should work. I'm not testing that right now but I will test it out over the weekend and let you know.

Also, could you show me how you have the resource owner schema currently set up with UUID? Also, wouldn't all primary id's in your project have to be UUID? I haven't worked with UUID and Ecto before.

Its up to the developer to choose which fields to be UUID and we don't need to have all primary keys as UUID.

[danschultzer]

Thanks @techgaun, I've updated #21 so if necessary, you can set individual schema definitions like so:

config :ex_oauth2_provider, ExOauth2Provider,
  oauth_access_grant_schema: MyApp.SchemaA,
  oauth_access_token_schema: MyApp.SchemaB,
  oauth_application_schema: MyApp.SchemaC

But I think for most use cases you'll have the same schema definitions for groups. So in the case of having a mix of int and uuid, you'll have two schema definitions that'll be used.

[techgaun]

@danschultzer I was actually thinking if we could just generate these schemas (https://github.com/danschultzer/ex_oauth2_provider/blob/master/lib/ex_oauth2_provider/oauth_applications/oauth_application.ex) to the users install just like this package generates controllers and views. For the grant schemas, I am pretty sure we can just stay with one schema instead of individual base schemas.

Also, because there is possibility of matching to id being in map (such as here:

ex_oauth2_provider/lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex

Line 191 in f9c7722

%{id: _} -> params

), this fix would probably be incomplete in terms of people being able to use non-standard column name in the owner schema.

[danschultzer]

Yeah, there was a lot of refactoring I wanted to do that's going to make this easier. All done now with #22, and now I've added tests for UUID tables in #21, and should hopefully work.

The following has been added:

• Mix task configuration for uuid (e.g. mix ex_oauth2_provider.install --uuid resource_owners)
• Dynamically check for belongs_to type so UUID can be used
• Test for part or all UUID tables

@techgaun let me know if this works for you. Creating the schema struct files would be the most flexible solution, but if this works I think this will be cleaner and easier to manage (e.g. version updates).

[danschultzer]

Just to make clear, and maybe add to the readme, this is how it works:

Only the resource owner requires :uuid as the primary key (and association)

Only the migration file needs to be modified or can be created with mix ex_oauth2_provider.install --uuid resource_owners. The belongs_to association will automatically figure out the field :type.

Database doesn't have auto incremental integer support

A schema file should be used similar to:

defmodule MyApp.Schema do
  @moduledoc false
  defmacro __using__(_) do
    quote do
      use Ecto.Schema
      @primary_key {:id, :binary_id, autogenerate: true}
      @foreign_key_type :binary_id
    end
  end
end

And configuration can be set with:

config :ex_oauth2_provider, ExOauth2Provider, app_schema: MyApp.Schema

The migration file should be created with:

mix ex_oauth2_provider.install --uuid all

[techgaun]

@danschultzer #22 looks pretty awesome. I tested #21 and it works fine. Thanks for getting those in and the #22 actually makes it so easy for me (& others) to use this package flexibly.

[danschultzer]

Thanks for the help @techgaun! v0.2.3 is on hex now 🎉

[danschultzer]

Quick update, v0.3.0 has been released after fixing the issue in #23. There's some minor changes to how the app should be configured: #24

Also, in case you're using phoenix_oauth2_provider, that library has been updated to v0.3.2 that now supports the --uuid argument too.