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

CSRF-Token on login form #5

Open
maberer opened this issue Dec 16, 2018 · 6 comments
Open

CSRF-Token on login form #5

maberer opened this issue Dec 16, 2018 · 6 comments

Comments

@maberer
Copy link
Contributor

maberer commented Dec 16, 2018

How can I provide a CSRF-Token to the user login form - there seems to be no way to provide
custom data to the login form...
https://github.com/rivo/users/blob/master/loginout.go#L31

@rivo
Copy link
Owner

rivo commented Mar 4, 2019

Apologies for the late response here, too. Is this still relevant to you?

You are correct, a CSRF-Token would be a good addition to the login form (the logout form as well). It should probably be built into this package. I'm not sure yet if we should use an extra cookie or add the token to the sessions package. If you have preferences, let me know.

@maberer
Copy link
Contributor Author

maberer commented Mar 20, 2019

Yeah the issue is still relevant... I have not yet though about where to place the cookie... have to think about that.... if you have an idea, please go ahead.

@maberer
Copy link
Contributor Author

maberer commented Nov 9, 2019

hi I just wanted to ask back on any ideas that might have come up...

I think we should separate the CSRF-Token cookie and the rivo session... in order to let people decide, what they want to use....

Speaking of preferences I would prefer to use something like
https://github.com/justinas/nosurf
for CSRF protection. The package seems to work well and is pretty simple.

The package is build as a middleware and thus could probably be used with rivo/users. The only problem is, that we have no way to access the data struct that is attached to the templates in order to provide the
token to the template e.g. context["token"] = nosurf.Token(r)...

More generally, I think it would be great, if a user would be able to provide his own data map for the templates that are handled by rivo/users. This way, it would be easy to provide the token to the template (e.g via. data["token"] = nosurf.Token(r))

Having a way to provide a data map to the template, would allow users to further customize the templates... For now, our only option is to change the html layout (but it is not possible to inject additional values to the template)....

The CSRF situation would be solvable with a more generic custom data injection mechanism.

I could think of something like hook functions, that are provided by the Config struct and called by the framework prior to rendering a template. The function could provide a data map that is then accessible from inside the templates... the prototype of the function could look something like:

func signupTmplDataHook(r *http.Request) map[string]interface{}

What do you think?

@maberer
Copy link
Contributor Author

maberer commented Nov 9, 2019

Or probably even simpler: in RenderPage() we could just extend the data object with everything that comes in e.g. context.extData. (just access context though the already available request object) Users could access the additional data as e.g. .extData within the template.

extData could be an interface{}, that is added to the context object by a middleware before the rivo/users handler is called.

Because the go context object is meant for request scoped data, it would probably be a good fit.

@maberer
Copy link
Contributor Author

maberer commented Nov 10, 2019

I implemented both solutions described above.

I found that the context solution works, but requires special consideration for go version prior to 1.7 because the context has been changed in this version....

Because of that, I went with the callback solution but broke it down to only one callback handler.
I guess this should be a reasonable solution.

Please review the referenced PR from above. Thanks

@maberer
Copy link
Contributor Author

maberer commented Nov 10, 2019

If the PR is accepted, I will update the docs.

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

2 participants