pushbits / server

A simple server for push notifications via Matrix (and a minimalistic alternative to Pushover and Gotify) 🚀📯
https://www.pushbits.io
ISC License
305 stars 18 forks source link

Support additional authentication methods - Oauth #29

Open CubicrootXYZ opened 3 years ago

CubicrootXYZ commented 3 years ago

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:

CubicrootXYZ commented 3 years ago

Regarding the longterm tokens you'd like to add, what are the high-level steps we need to implement this?

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.

CubicrootXYZ commented 3 years ago

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.

eikendev commented 3 years ago

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.

CubicrootXYZ commented 3 years ago

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).

CubicrootXYZ commented 3 years ago

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.

CubicrootXYZ commented 3 years ago

We should add state and challenge parameters to further secure the authentication and add a config option to restrict the redirect url.

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.

CubicrootXYZ commented 3 years ago

We should add ~state~ and challenge parameters to further secure the authentication and add a config option to restrict the redirect url.

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.