gobuffalo / buffalo-auth

Buffalo auth plugin helps adding username password authentication to your app
https://gobuffalo.io
MIT License
41 stars 28 forks source link

Proposed change to `buffalo g auth` default template in `app.go` #67

Closed kasuskasus1 closed 1 year ago

kasuskasus1 commented 4 years ago

Propose the following changes to handlers which are being automatically added to app.go file. Currently lines are:

//AuthMiddlewares
app.Use(SetCurrentUser)
app.Use(Authorize)

//Routes for Auth
auth := app.Group("/auth")

auth.GET("/", AuthLanding)
auth.GET("/new", AuthNew)
auth.POST("/", AuthCreate)
auth.DELETE("/", AuthDestroy)
auth.Middleware.Skip(Authorize, AuthLanding, AuthNew, AuthCreate)

//Routes for User registration
users := app.Group("/users")

users.GET("/new", UsersNew)
users.POST("/", UsersCreate)
users.Middleware.Remove(Authorize)

In order to default HomeHandler to work properly suggest adding a middleware skip:

//AuthMiddlewares
app.Use(SetCurrentUser)
app.Use(Authorize)
app.Middleware.Skip(Authorize, HomeHandler)

Also login behaviour (create user -> try to login) - causes error ('auth/new does not exist'). Suggest replacing auth.POST("/", AuthCreate) with auth.POST("/new", AuthCreate)

Unfortunately, I am a newbie so still do not know how to change tests to make them pass on these changes...

rayjanoka commented 2 years ago

This helped!

//AuthMiddlewares
app.Use(SetCurrentUser)
app.Use(Authorize)
app.Middleware.Skip(Authorize, HomeHandler)

//Routes for Auth
auth := app.Group("/auth")
auth.GET("/", AuthLanding)
auth.GET("/new", AuthNew)
auth.POST("/new", AuthCreate)
auth.DELETE("/", AuthDestroy)
auth.Middleware.Skip(Authorize, AuthLanding, AuthNew, AuthCreate)

//Routes for User registration
users := app.Group("/users")
users.GET("/new", UsersNew)
users.POST("/", UsersCreate)
users.Middleware.Remove(Authorize)
sio4 commented 2 years ago

I do not fully understand the last part of this issue regarding auth.POST("/new", AuthCreate), my test code worked fine with the current repository version. Could you please provide me with some more details?

I know this issue was filed so long time ago, maybe you are not able to follow up on this anymore. I will however check the status of this plugin and will fix the issues. (actually, we did not release the latest code so buffalo plugin install will not use the latest version for now and we are working on it.)

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

sio4 commented 1 year ago

Thanks for filing an issue.

For the first part, The choice is not by the tool but should be done by users since allowing to access the home page without authentication could be fine for some applications but not fine for others. The scaffolded code does not intend to be the final version for your own application, but it just shows the possibility of use cases of the middleware as authentication/authorization methods.

In order to default HomeHandler to work properly suggest adding a middleware skip:

//AuthMiddlewares
app.Use(SetCurrentUser)
app.Use(Authorize)
app.Middleware.Skip(Authorize, HomeHandler)

For the second topic, actually, I am not sure what is the problem you wanted to fix. Also, I could not find a meaningful difference between the suggested and the original sentences.

Also login behaviour (create user -> try to login) - causes error ('auth/new does not exist'). Suggest replacing auth.POST("/", AuthCreate) with auth.POST("/new", AuthCreate)

I am going to close this issue for now, but please feel free to reopen it with additional explanations so we can continue the discussion!