Clem-Fern / rtabby-web-api

Tabby Web API for Config Sync
GNU Affero General Public License v3.0
61 stars 4 forks source link

Add third-party login #10

Closed EkkoG closed 11 months ago

EkkoG commented 11 months ago

For now, get GitHub user info works fine, I need to imp user table next.

Platform support

Related issue https://github.com/Clem-Fern/rtabby-web-api/issues/9

EkkoG commented 11 months ago

For now, token will print at the page of GitHub callback, it's better to redirect to another page without the code given by GitHub, because if not, browser will create a history record with a URL contains the code, but I don't know how to pass the token to another page safety, @Clem-Fern do you have any idea?

EkkoG commented 11 months ago

I have saved the token to cookie and redirect to the main login page now.

EkkoG commented 11 months ago

Finally, I deploy an instance of this PR on my own server https://tabby-sync.imciel.com/

EkkoG commented 11 months ago

After full test, configs API access auth forgot to be changed, for now I do not have an idea to access the db at bearer_auth_validator method, alternative solution is to maintain a mutable cache global, it seems not easy as other languages, I will keep going to do this work next.

Clem-Fern commented 11 months ago

Hey @EkkoG,

Thank you for working on this. Globally, it seems great.

Small nitpicks, I would have liked to make this feature optional.

Based on what you've done, I added an exemple about how you can handle bearer auth with db access. https://github.com/Clem-Fern/rtabby-web-api/blob/a6ef4b6480bf8cbec2289f275aeb5d90a92b2206/src/auth.rs#L19-L33

Also, I think token field in users table should be unique.

Secondly, i think it could be great to have more oauth provider like gitlab or microsoft. I will try to work on this.

EkkoG commented 11 months ago

Token is UNIQUE now.

If GitHub auth have done, add other auth method will very easy, just get user info and generate a token to user, no need to change the config API things.

Make this feature optional is great but a little complicate, the feature changes the Dockerfile and migration SQL, the code can be featurize, but how to make Dockerfile and SQL featurize? If add more files like Dockerfile-sqlite, there will have a lot of files, at least four Dockerfile.

EkkoG commented 11 months ago

Please note that I have implemented Google and GitLab login @Clem-Fern

EkkoG commented 11 months ago

Now all tasks have finished.

I had made a big change of the Dockerfile and docker-compose files, now each of them only has one file, features can be enabled by docker build-args, if you feel this change is not very good, I can remove relate commits from this PR.

For why merge two of the docker-compose.yml file, because we can start by use service name, like

docker-compose up rtabby-sqlite
EkkoG commented 11 months ago

One thing needs to discuss is, do you think it need to support bind an account from another platform to current user? If it's need, now it's better to change the user table, and add a third-party user table to store the platform info, one user can link to multiple user info third-party user table.

Clem-Fern commented 11 months ago

Hey,

Again, thank's a lot for your great work.

About docker compose, I've always separate service and image in different files before. It was my way to work but I honestly never wondered if it was the right way to do. Do you think it is more user friendly with only one docker compose file ?

I will test all this in the next few days. In the same time, I'll try to write some docs about it.

EkkoG commented 11 months ago

I don't think there is an absolutely correct way to use it, for only one file it's easy to maintain and document the feature.

I moved the login code to a separate module, just few lines code add with feature flag at auth.rs and routes/user.rs because these codes depend on the API call in main app.

EkkoG commented 11 months ago

I think it not necessary to support bind multiple third-party account to one token now, even if it need to support in the future, we can add a migration, move the platform info to a separate table, then allow user to bind another account.

EkkoG commented 11 months ago

All the code has updated on my own server. https://tabby-sync.imciel.com/

Clem-Fern commented 11 months ago

Writing a bit of docs about this feature and then merging dev into master to release a new version.

Thank's again ;)