Support additional authentication methods - Oauth #29
Conversation
added a proof of concept for oauth2
internal/router/router.go
Outdated
| { | ||
| oauthGroup.GET("/token", ginserver.HandleTokenRequest) | ||
| oauthGroup.GET("/auth", ginserver.HandleAuthorizeRequest) | ||
| oauthGroup.GET("/tokeninfo", auth.RequireValidAuthentication(), oauth.GetTokenInfo) |
There was a problem hiding this comment.
Add an endpoint for retreiving longterm tokens that last forever or for a few years to be able to let other applications manage pushbits.
There was a problem hiding this comment.
added /longtermtoken with a token that is valid for 5 years. But I am not quite happy with the data formats, as the use of url encoding and json encoding is not very consistent. Was not able to get it to work with url encoding.
There was a problem hiding this comment.
Can you elaborate on the "not very consistent" part?
There was a problem hiding this comment.
/auth and /token need url encoded data (e.g. client_id=XXXXX&client_secret=yyyyy). /revoke and /longtermtoken expect JSON formated data.
There was a problem hiding this comment.
Thanks @CubicrootXYZ, well done!
I gave this a first pass, will probably need a second pass on the weekend. Don't want to rush this as authentication is critical, but I definitely feel this is a step in the right direction. I especially like the modular approach; makes it more readable indeed!
The complexity of the setup is surprising, wasn't aware so much boilerplate will be needed. The ginserver examples make it look so easy, but that's presumably because they don't provide equal functionality.
Regarding the longterm tokens you'd like to add, what are the high-level steps we need to implement this?
| // UserAuthHandler extracts user information from an auth request | ||
| func (a *AuthHandler) UserAuthHandler() server.UserAuthorizationHandler { | ||
| return func(w http.ResponseWriter, r *http.Request) (string, error) { | ||
| username := r.URL.Query().Get("username") |
There was a problem hiding this comment.
I think we should put the credentails into a POST body instead of GET parameters, because otherwise they will be visible in server logs.
There was a problem hiding this comment.
Moved /auth and /token to POST.
There was a problem hiding this comment.
Thank you. But isn't the data still read from the query parameters? In essence, when someone does a call to let's say https://pushbits.example.com/oauth/token?mysecret=foobar, a web server will log the full URL, meaning the credential is stored somewhere in the logs. Instead what we can do is use form parameters or pass a JSON body, which is typically not logged by a web server.
There was a problem hiding this comment.
The authentication library can handle both, so that's why I just moved to Post, you simply can move the data from the url to the body and it will work. I just forgot my own UserAuthHandler, adapted it to also use "FormValue" now - which get's data either from url or body. Conclusion: in the current state you can do both, pass the data in the url or only in the body.
E.g.
curl "https://push.example.con/oauth2/auth" -d "client_id=XXXXXXXXX&username=admin&password=YYYYYYYY&response_type=code" -X POST -i
I think we need to make a Handler that simply calls oauth2's GenerateAccessToken function and then return the generated token to the user. I'd only allow the generation for the currently logged in user. |
|
I'd like to open a discussion about error messages. Currently all authentication related errors will be displayed as internal server errors to the client with a generic error message. The authentication library itself used to differentiate there a bit more, but I can not use their error handling without loosing the ability to log all errors to the console (for debugging). I would at least change the internal server errors to unauthenticated errors. |
Is that so the errors are more consistent or is there another issue I'm overlooking? If the former is the case, happy to use other error codes. Only thing I'd look out for is that we don't expose too much detail. |
Errors are currently very consistent - always HTTP 500 - I'd like to at least change that to a 403 or maybe just reenable the build in error handler with a bit more detailed errors (still very generic). |
Digged a little deeper into that. The errors are still as desired from the oauth library, the error handler I inserted in the library just handles internal errors, so that is fine and does not need any changes. |
|
We should add Edit: state is already implemented - challenge not. Redirects are bound to the given redirect url in the config => enable multiple clients for multiple redirect urls. |
Implemented multiple clients as the clients are restricted to the redirect url that is given in the config file. With this I also came across the issue that clients were not deleted in the database when removing from the config file. This is solved now too. |
Added support for additional authentication methods and implemented oauth with JWT tokens.
Take a look in the changes in the README file to check out how authentication with oauth is working.
Currently supported token storages: