aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
774 stars 56 forks source link

Make the `auth_module` less coupled to controllers #28

Closed thojanssens closed 4 years ago

thojanssens commented 4 years ago

The auth_module is designed to work for controllers only. I was thinking that it is possible to divide it into two modules:

One can say, just modify the code according to the type of app you have. But some have multiple APIs (use of controllers, use of Absinthe, etc.). This then doesn't just require to modify a few lines of code, but to change the design for less coupling.

Just an idea.

josevalim commented 4 years ago

Similar discussion has come up elsewhere and it is impossible for us to generate code that will work on all situations. Every time we make the code more generic for future usage, we make the code harder to understand for the simple cases. It is your responsibility to take the generated content further.

For example, imagine we change the code somehow to make it easier to work with APIs and LiveView. Now someone wants to add Absinthe support and it is much harder, because instead of the logic being neatly encapsulated in one place, it is spread out over multiple places, making it harder to understand how the pieces fit together.

My hope is that if you want to change the generators to absinthe (for example), we will have blog posts and articles about it in the future, as the generators become a standard. Or maybe people will even work on their own extensions.

But in terms of principle, I am :-1: on making the generators more and more generic.

thojanssens commented 4 years ago

Thank you for sharing your opinion. I was thinking about a module with these functions: get_user_token/1 (get token from session cookie or from remember-me cookie) put_user_token/3 (write token in session cookie and maybe in remember-me cookie) delete_user_token/2

This would be a common base for all the types of apps.

Then another module would be added on top of it, because it just happens that the generated app has server-side rendering with controllers in mind.

You have already expressed your principle, so this issue may be closed if no one has any further comments.

josevalim commented 4 years ago

generate_session_token, get_user_by_session_token, and delete_session_token in the context are the low-level functions that you want.

thojanssens commented 4 years ago

Sorry, these functions I wrote (rather extracted from the auth_module) operate on the cookie (using get_session/put_session/...).

The functions in the context that you mention operate on the database.

The auth_module manages the token in the cookie + redirects + contains a plug. I extracted the management of the token in the cookie, as a base for all types of apps (delete_user_token/2 is a little tricky).

josevalim commented 4 years ago

Right but even cookies are not general enough. Some SPAs prefer to avoid cookies altogether and use headers. You can't set cookies in LVs either, so you need a separate mechanism. So I stand with my vote of keeping things simple since it is always hard to guess. :)

thojanssens commented 4 years ago

SPAs that don't want to use cookies wouldn't need this module at all though; I'd imagine they will only use the context functions. So it is not like the module would need to be adapted in this case, it will just be ignored. Just wanted to mention that important difference:-D

LV however I have no idea how it works yet:)

The suggestion was to have code to be easy to use as-is for server-side rendered application and SPAs working with cookies (and they always should use cookies, except if working as third party used by different domains and such).

But I understand here that for some, it might not be a strong enough reason to apply some code changes.

josevalim commented 4 years ago

The suggestion was to have code to be easy to use as-is for server-side rendered application and SPAs working with cookies

Right, that's a good enough solution for you, but my point is exactly that someone could be making the point that cookie-based is not enough and we should further make the generators extensible even for people doing header-based auth. It is a sloppery slope and I personally have no interest in going down the road of making the generator generic. It is easier to say that it works for the generated code rather than define an arbitrary line on what is supported.

thojanssens commented 4 years ago

Thank you for your input! I am closing this issue.