immich-app / immich

High performance self-hosted photo and video management solution.
https://immich.app
GNU Affero General Public License v3.0
35.37k stars 1.67k forks source link

OAuth2.0/OIDC implementation #207

Closed EnricoBilla closed 1 year ago

EnricoBilla commented 2 years ago

Hi all! I was interested in implementing OAuth2.0/OIDC login in Immich as discussed in #33. I gave it a try and after all I adapted the backend to use OAuth2 in combination with the local authentication that was already implemented; here are the details for now.

Instructions removed, please take a look at this message for updated instructions


I've marked this as a draft pull request because I need to work on the mobile app and on the web interface before it's fully working. Before working on that tho, I wanted to discuss with you some topics.

First of all is the expected behaviour when migrating an already existing instance of Immich to use OAuth2. I was thinking that when an user first access to Immich with a valid OAuth2 access token, his local account would be disabled and he has to continue using the OAuth from that point on. What do you think about that?

Second is if we should keep the possibility of having both local and OAuth authentication working together at all. I kept them as most separate as possible to be fully flexible, but being very easy to fix I dismissed the problem for later.

Last one is about the admin account. For what I've seen from latest commits an admin account has been created in order to create new users and while using OAuth2 this function is not needed. Is it planned to have more admin functions so that it does make sense to implement a way of distinguishing admin accounts with OAuth (through scopes for example)?

Let me know what you think about this πŸ˜„

alextran1502 commented 2 years ago

Firstly, thank you for opening a PR. I appreciate you spending the time to do the research and implement this feature. This PR for sure increases the quality of the app significantly

Below are my answer/question to your concerns

Note that for now the websocket authorization is not working as I have to figure out a clean way to do it.

For the WebSocket, we are also using the access token, after getting the access token from the OAuth, I assume it can still be used as the current flow when opening the socket. Can you elaborate on your concern?

First of all is the expected behaviour when migrating an already existing instance of Immich to use OAuth2. I was thinking that when a user first access to Immich with a valid OAuth2 access token, his local account would be disabled and he has to continue using the OAuth from that point on. What do you think about that?

Second is if we should keep the possibility of having both local and OAuth authentication working together at all. I kept them as most separate as possible to be fully flexible, but being very easy to fix I dismissed the problem for later.

I don't have a full understanding of how OAuth flow works, what are the pros and cons of having both OAuth properties integrate into the current user information?

Last one is about the admin account. For what I've seen from latest commits an admin account has been created in order to create new users and while using OAuth2 this function is not needed. Is it planned to have more admin functions so that it does make sense to implement a way of distinguishing admin accounts with OAuth (through scopes for example)?

The admin account will be there to administer the server. There are plans to have additional settings that can be set through the admin interface (Like enable transcoding, which set of video transcoding should the server use...etc). So I would like the admin still can control who can access the server.

Do you have any good resources for developing an OAuth system? And what OAuth that people can self-hosted, I would like to spin up one to test :)

Note

This PR https://github.com/alextran1502/immich/pull/206 would change the project of the server folder quite heavily. I suggest you either merge the PR to your branch or wait until I merge it to main to update your branch.

EnricoBilla commented 2 years ago

For the WebSocket, we are also using the access token, after getting the access token from the OAuth, I assume it can still be used as the current flow when opening the socket. Can you elaborate on your concern?

No no, I'm not concerned, I just have to research a way to use the already implemented AuthGuard with websockets instead. I will work on that!

I don't have a full understanding of how OAuth flow works, what are the pros and cons of having both OAuth properties integrate into the current user information?

The general idea here is that with OAuth2.0/OIDC the backend doesn't have to handle authentication and authorization. For this reason once the user is logged with the token generated from your OAuth server, the backend validates that token and fully trusts what's in the payload. So for example it would be possible to configure a user called enrico on the OAuth server, when I log into Immich I get redirect to my OAuth server to insert user/password, after the successful login I retrieve the token to pass to Immich in the API calls. Immich now is reading that token and validating it against the OAuth server.

What I was wondering is if we want to allow an instance of Immich to use both JWT and OAuth2 together, or if we want the user to pick an authentication type and use only that.

The admin account will be there to administer the server. There are plans to have additional settings that can be set through the admin interface (Like enable transcoding, which set of video transcoding should the server use...etc). So I would like the admin still can control who can access the server.

Okay, I will work on that too!

This PR https://github.com/alextran1502/immich/pull/206 would change the project of the server folder quite heavily. I suggest you either merge the PR to your branch or wait until I merge it to main to update your branch.

Thanks for letting me know, I still have a lot to do so I will wait and merge from main.

Do you have any good resources for developing an OAuth system?

I found auth0 to be very good to start with. It does explain all the concepts needed and it gives examples on how to implement it. Maybe you can start reading from here.

And what OAuth that people can self-hosted, I would like to spin up one to test :)

For testing I'm using Authentik which is straight forward to spin up in docker and the configuration is easy too. You just have to create a new application and a new provider associated with the application. From the provider page you can retrieve all the info you have to put in the environment variables.

Tell me if something is not clear!

EnricoBilla commented 2 years ago

Hi again, with the last commit I changed the validation flow. To test now you should generate an id_token in OIDCDebugger and use that in the bearer token... It's very likely I'll need to change some config environment variable too, I will keep updated the first message with the correct intruction

zackpollard commented 2 years ago

This would be an awesome addition, although I don't have a self-hosted OAuth setup yet, I definitely want one for these kind of applications. To chip in on some of the questions:

What I was wondering is if we want to allow an instance of Immich to use both JWT and OAuth2 together, or if we want the user to pick an authentication type and use only that.

Without looking at any of the code, I imagine this would result in added complexity and probably isn't necessary. However, if you don't believe so, then maybe it's worthwhile having for people who want users who aren't fully fledged OAuth users. Whichever way we go here though, I think that the admin account should have the option of logging in either way, incase something with OAuth breaks and you need a backup to get into the server. This is especially necessary as longer term it would be good to move away from using environment variables for all of these config options and instead pushing them into the SQL server with all the options configurable from the admin panel.

EnricoBilla commented 2 years ago

@zackpollard I agree on the possibility of having both authentication type enabled together. The approach I followed now is that the user can enable OAuth through env var, if OAuth is enabled then it's possible to disable the local login with LOCAL_USERS_DISABLE (or similar, I don't remember rn). Maybe it would be better to disallow the creation of new local users but leave enabled the login for already registered ones?

I don't think moving the configuration to a SQL database would be a good choice, in particular because it would add difficulties with the provisioning of the docker container. But that's a problem for the future I guess haha

EnricoBilla commented 1 year ago

Hi all, finally the mobile app and server are ready to be used with OAuth2/OIDC. I'm sure there is some polishing to do and whole web client is not compatible yet with this authentication, but here I'm writing a step by step guide to manually test this PR.

First of all you should have your own OIDC server set up, if you don't then you can find how to setup Authentik here (you don't have to setup email and GeoIP).

Once you have Authentik setup make sure it's exposed through HTTPS (it's needed by the mobile app library that handles OAuth2, otherwise it won't work).

Then you have to setup a provider, so in Authentik go in Applications > Providers and create a new one: in the first page select OAuth2/OpenID Provider, then in the second page give it a name and change Confidential to Public. After that you have to create a new application in Authentik, go to Applications > Applications and create a new one: pick a name and a slug, select the provider you create just before and leave everything else unchanged. You need to add some users, so go to Directory > Users and create all the users you want (just make sure to always enter an email). For each user you create, click Impersonate and in the user setting change the password to the one you want.

If you want to create admin users, you should create a new group called immich_admin and add the user to that group.

Before starting Immich make sure you add the environment variables needed:

OAUTH2_ENABLE=true
LOCAL_USERS_DISABLE=false #not needed
OAUTH2_ISSUER=https://example.com/application/o/immich/
OAUTH2_CLIENT_ID=your_oauth2_client_id
OAUTH2_CERTIFICATE=#here goes the bae64 encoded PEM certificate of your OIDC provider

You can find the issuer URL and the client ID in the provider page in Authentik, the certificate is in System > Certificates (download the certificate, base64-encode the content of the file).

That's all you need to setup to use OAuth2/OIDC, so now you can start your Immich server and open the application.

Leave the user and password blank, just enter the Immich endpoint and click login. You should be redirected to the login page of Authentik, enter there username and password.

If the login is successful then you should be redirected back to the mobile application, from there you can use everything as before.

Please try it out and let me know if you have any feedback! πŸ˜„

EnricoBilla commented 1 year ago

Hi again, so here it is Immich with full OAuth2/OIDC support. Backend, mobile app and web all support OAuth2 authentication now. The setup has slightly changed, just a different env var to set (you can find all the instruction in the message above).

In this iteration I implemented the authentication for the webapp but I've never used Svelte before, so I kindly ask you to take a look at the fantastic code I wrote. For now I can only guarantee that it works, but I feel it violates all possible best practices. So please take a look at the new code since the moment this PR could be finally reviewed to be merged is coming closer and closer! Thanks! πŸ˜„

alextran1502 commented 1 year ago

Hey man just to let you know that I appreciate the work. I apologize there have been other issues that need to be resolved before fully setting this up to do a thorough testing.

EnricoBilla commented 1 year ago

Hi Alex, thanks for the appreciation!

Right now I'm most concerned about two things:

1) the web frontend code: I haven't used Svelte and I don't like the code I wrote, the OAuth2 part is working tho so it shouldn't be too difficult to refactor for someone who knows the framework

2) the backwards compatibility of the mobile app with the backend: as of now the authentication for the websocket has slightly changed and it's the only breaking change. For this reason I was thinking of keeping both the old and new authentications for some releases, and in a month or two remove the compatibility code to keep the new one only. What do you think?

zackpollard commented 1 year ago

2. the backwards compatibility of the mobile app with the backend: as of now the authentication for the websocket has slightly changed and it's the only breaking change. For this reason I was thinking of keeping both the old and new authentications for some releases, and in a month or two remove the compatibility code to keep the new one only. What do you think?

Hey, would also like to extend the thanks for working in this, it's a big piece of work to implement and will be a great addition.

As for the 2nd question, I think it's fine to release breaking changes right now. Immich is still in a period of fast development and has not hit its first major release milestone yet, so we have been releasing breaking changes without too much thought. Currently we are only really focussed on keeping the latest app version working with latest server version. Alex can confirm, but we've not been holding back on releasing breaking changes yet as maintaining two lots of compatible code will slow down feature development and push back the first initial release.

EnricoBilla commented 1 year ago

Thanks @zackpollard for the answer, I’ll keep that in mind!

EnricoBilla commented 1 year ago

Hi @alextran1502, I just merged from main all the changes. I've got to admit that it's becoming really difficult to keep up with the changes since I also changed a lot of stuff on my end. I'd like to merge as soon as possible these changes 😬

Since I'm most confident on the backend and on the flutter app, if you want I can create a new PR for those only. Then we will merge the web frontend when you have more time to check it thoroughly. Sorry for asking but it's really time consuming having to merge conflicting changes. Thanks for your comprehension! 😁

alextran1502 commented 1 year ago

@EnricoBilla I will try to have this merge this weekend. Thank you

alextran1502 commented 1 year ago

I've set it up and my starting point is from the web.

My networking spec is as followed

Authentik and Immich are running from a remote development machine with IP 192.168.1.216.

Immich-web can be accessed at

http://192.168.1.216:2283

Authentik can be accessed with the two IP addresses

  1. http://192.168.1.216:9000
  2. https://192.168.1.216:9443

I can ping immich-server at http://192.168.1.216:2283/api/server-info/ping

Issue 1

After creating a normal user on the web and navigating to the login page. Upon clicking on the Oauth2 Login, the page stayed there and I see this in the console

image

And then clicking again on the button I see this

image

This is my .env setup

OAUTH2_ENABLE=true
LOCAL_USERS_DISABLE=false #not needed
OAUTH2_ISSUER=https://192.168.1.216:9443/application/o/immich-oauth/
OAUTH2_CLIENT_ID=2309773c0fc9538f2dc5c290f600644dd4f37ae3
OAUTH2_CERTIFICATE=-----BEGIN CERTIFICATE----<content>-----END CERTIFICATE-----
EnricoBilla commented 1 year ago

Re: Issue 1

@alextran1502, that issue is not related to Immich but to Authentik. Please go in the Provider settings and verify that the "Redirect URIs/Origin" contains https://192.168.1.216:9443. The CORS error already happened to me before and it was an issue with this.

Let me know if it works!

Also note that the OAUTH2_CERTIFICATE env var need to be base64 encoded. I'd like to use a better solution for this, but didn't find anything :(

alextran1502 commented 1 year ago

@EnricoBilla How do I generate the base64 encoded content from the .pem file? πŸ˜… ?

EnricoBilla commented 1 year ago

@alextran1502 on Linux you can use cat file.pem | base64. Otherwise you can just use an online base64 encoder, the PEM certificate doesn't include a private key so no worries using external tools.

alextran1502 commented 1 year ago

Hello @EnricoBilla

Here is some update.

I still have a problem using OAuth on the web, still, I haven't tried it on mobile yet.

Here is the error message when click on OAuth2 Login on the web at from http://192.168.1.216:2283/auth/login

Screen Shot 2022-07-03 at 00 31 33

Here is my application provider

Screen Shot 2022-07-03 at 00 32 12

Here is my .env

OAUTH2_ENABLE=true
LOCAL_USERS_DISABLE=false #not needed
OAUTH2_ISSUER=https://192.168.1.216:9443/application/o/immich-oauth/
OAUTH2_CLIENT_ID=2309773c0fc9538f2dc5c290f600644dd4f37ae3
OAUTH2_CERTIFICATE=base-64-pem-using-cat-file-|-base64

What should I do in this case?

EnricoBilla commented 1 year ago

Hi @alextran1502, are you using a self signed TLS certificate?

I guess you need one from a valid CA. I'm sorry but I didn't know about this requirement, it's a check made by the library.

alextran1502 commented 1 year ago

Hmm I didn't use any self-signed certificate since I access it from my local network. What did you have to do resolve this issue on your setup?

EnricoBilla commented 1 year ago

I used a valid TLS certificate generated from Let's Encrypt. I use a wildcard certificate for my domain, so I don't have to expose test services on the internet. If you want I can give you privately admin access to my test Authentik.

alextran1502 commented 1 year ago

So you will need to generate the TLS certificate for the Authentik instance even though you are accessing it from the local network?

EnricoBilla commented 1 year ago

If you want a straightforward way I think yes. Otherwise, and I'm taking guesses here, you should try to add the CA certificate to the root certificates on your operating system or browser

Also note that having a TLS certificate doesn't necessarily mean that you have to expose the services to the Internet. You can obtain a TLS certificate with a DNS01 challenge that does not require exposing anything publicly

alextran1502 commented 1 year ago

I see, I think I can reverse proxy with https connection to my Authentik instance. Probably will resolve this issue. If I do that, do I have to change anything in the provider?

EnricoBilla commented 1 year ago

Perfect, you need to always have the URL you are accessing Authentik from in the redirect URIs in the provider. So in this case you should add your external domain in the redirect URIs

alextran1502 commented 1 year ago

Hey, @EnricoBilla there will be some changes in the backend that I and the team will have to circle back and look at this PR since this is a big change to the project and it relates to authentication/security.

I will make a branch for this so the team can work on this feature to help you not have to update the PR to match the upstream branch.

You are welcome to let this open and keep working on that, I just don't want to discourage you by letting you keep having to update the branch without seeing it merge into upstream.

Thank you for your understanding.

EnricoBilla commented 1 year ago

Hi @alextran1502 Thanks for letting me know, actually I feel like all the features are implemented for this PR. I had to fix some bugs on the mobile app to skip the login page when the user is already authenticated.

Btw, did you manage to solve the issue we discussed in the previous messages?

PixelJonas commented 1 year ago

Is there anything I can do to support this PR?

EnricoBilla commented 1 year ago

@PixelJonas I really don't know, I still want and would like to contribute to Immich with this PR. Tho I don't know what's the developer plan for the first release.

Backend and mobile app was completely working and I consider them finished. The web frontend was working, but since I had zero experience with Svelte, I really don't like the code there. If you are more experienced in this, it would be awesome if you could rewrite the implementation there.

In any case, the main branch was merged with this PR more than two months ago, I guess there's some work to do to merge from upstream now. On the other hand I'd prefer to merge only once when I know this PR is going to be merged soon.

FranzSw commented 1 year ago

I think @alextran1502 has to give a go / update here. I'd also offer support in resolving conflicts and rewriting parts of the code, if necessary.

alextran1502 commented 1 year ago

@FranzSw we just discussed this yesterday internally and it is time to bring this upstream after resolving all of the performance issue on the web and mobile. So I expect to work on this in the next couple releases

christiaangoossens commented 1 year ago

@EnricoBilla Quick note before this goes upstream, you should get the certificate public key from the JWKS, which can be found in the discovery document. If you refresh the JWKS once in a while this will also automatically enable certificate renewal for you. Might save some hassle with providers that rotate their certs ;).

Feel free to ask any questions you may have and good luck!

EnricoBilla commented 1 year ago

@christiaangoossens Thanks for having taken a look at this PR carefully! I didn't know that endpoint existed 😁 Do you know if there is any best practice on how to handle the refresh of the public key? Like daily, weekly or every time the token has an invalid signature (?).

Thanks again, I don't have time to fix this right now but I'll surely will before this PR is merged into master!

christiaangoossens commented 1 year ago

@EnricoBilla No problem, I actually quite like that you ask for just the issuer and client id, while using PKCE instead of doing the 'naive-approach' client_secret! Very well done!

I would do a combination of refreshing daily and refreshing on the first time that the token is invalid. This makes sure that old keys get removed, but that a new key works immediately. Also note that the JWKS might include multiple keys, you should match against all of them: any of them might match. This is the recommended way of key rollover as well, use both keys for a while and then remove the old one.

I saw some other things in the PR as well:

christiaangoossens commented 1 year ago

For the WebSocket, we are also using the access token, after getting the access token from the OAuth, I assume it can still be used as the current flow when opening the socket. Can you elaborate on your concern?

No no, I'm not concerned, I just have to research a way to use the already implemented AuthGuard with websockets instead. I will work on that!

I don't have a full understanding of how OAuth flow works, what are the pros and cons of having both OAuth properties integrate into the current user information?

The general idea here is that with OAuth2.0/OIDC the backend doesn't have to handle authentication and authorization. For this reason once the user is logged with the token generated from your OAuth server, the backend validates that token and fully trusts what's in the payload. So for example it would be possible to configure a user called enrico on the OAuth server, when I log into Immich I get redirect to my OAuth server to insert user/password, after the successful login I retrieve the token to pass to Immich in the API calls. Immich now is reading that token and validating it against the OAuth server.

What I was wondering is if we want to allow an instance of Immich to use both JWT and OAuth2 together, or if we want the user to pick an authentication type and use only that.

The admin account will be there to administer the server. There are plans to have additional settings that can be set through the admin interface (Like enable transcoding, which set of video transcoding should the server use...etc). So I would like the admin still can control who can access the server.

Okay, I will work on that too!

This PR #206 would change the project of the server folder quite heavily. I suggest you either merge the PR to your branch or wait until I merge it to main to update your branch.

Thanks for letting me know, I still have a lot to do so I will wait and merge from main.

Do you have any good resources for developing an OAuth system?

I found auth0 to be very good to start with. It does explain all the concepts needed and it gives examples on how to implement it. Maybe you can start reading from here.

And what OAuth that people can self-hosted, I would like to spin up one to test :)

For testing I'm using Authentik which is straight forward to spin up in docker and the configuration is easy too. You just have to create a new application and a new provider associated with the application. From the provider page you can retrieve all the info you have to put in the environment variables.

Tell me if something is not clear!

@EnricoBilla This comment might not be useful anymore as you have already built it, but I just wanted to note on it as well from an architectural perspective.

You have chosen here to use the access_tokens from the OpenID Connect server for "internal" authorization as well (to replace Immich tokens), right? That might not always be the best architecture to go with, as you might want to control your tokens and its content. Some OpenID Connect servers might not even implement access_tokens at all. In this case, I would have chosen to implement the OpenID Providers just as an authentication provider, leaving the entire authorization part alone. Essentially, the OIDC Provider just plays the role of the username & password field in that case, after which you still generate a Immich token, the same one you would have if you used a password approach.

EnricoBilla commented 1 year ago

@EnricoBilla No problem, I actually quite like that you ask for just the issuer and client id, while using PKCE instead of doing the 'naive-approach' client_secret! Very well done!

I would do a combination of refreshing daily and refreshing on the first time that the token is invalid. This makes sure that old keys get removed, but that a new key works immediately. Also note that the JWKS might include multiple keys, you should match against all of them: any of them might match. This is the recommended way of key rollover as well, use both keys for a while and then remove the old one.

Yes, I was thinking the same but I preferred to ask anyway. Nice that multiple public keys can be included but I'm not sure the passport strategy can handle multiple keys, I need to test it.

I saw some other things in the PR as well:

Yes I was aware of that limitation. Maybe we can use the local DB when groups aren't available.

  • I am a fan of reusing an existing user if the email matches, which allows people to move to SSO, but you might want to implement some sort of check for that or make it configurable. Otherwise, especially if local auth has 2FA, you could use it as a bypass.

What kind of bypass are you thinking? Could you explain?

Thanks again!

jrasm91 commented 1 year ago

Worth mentioning that I took a stab at something similar in #884.

It's a different approach but it works pretty well IMO.

christiaangoossens commented 1 year ago

@EnricoBilla If you match up with existing users by email, and - suppose that - the local user login has 2FA, while the SSO provider does not, you could use the SSO version to login to the account, bypassing the local user login 2FA, as both will still work. Here's a real life example at CloudFlare: https://community.cloudflare.com/t/new-sign-in-with-apple-button-bypasses-2fa-works-on-existing-accounts/389163

@jrasm91 It sounds like you took the alternative approach that I would have done as well! From a security standpoint, I would be more in favor of that route.

EnricoBilla commented 1 year ago

@EnricoBilla If you match up with existing users by email, and - suppose that - the local user login has 2FA, while the SSO provider does not, you could use the SSO version to login to the account, bypassing the local user login 2FA, as both will still work. Here's a real life example at CloudFlare: https://community.cloudflare.com/t/new-sign-in-with-apple-button-bypasses-2fa-works-on-existing-accounts/389163

Ohh okay, but in this case OAuth is a different method of authentication in parallel with the local authentication. But I can understand the issue here. I also missed the update of Immich in the last months, has 2FA been implemented?

@jrasm91 It sounds like you took the alternative approach that I would have done as well! From a security standpoint, I would be more in favor of that route.

Well... I didn't know another PR was going to address SSO. If that's the preferred route I think I can close this PR then (?). I cannot hide my little disappointment here because this PR has been open since June and edits could have been discussed here without opening another PR, but anyway...

jrasm91 commented 1 year ago

@EnricoBilla I only recently opened the PR (like 7 days ago), and I did so with the intent to provide an alternative solution, which I assumed we'd be able to discussion. I thought it would be easier to explain if I had a working example. I've implemented OAuth like this a few times and it didn't take took long to put it together, so I went with that approach.

I'm sorry if this came off as undermining your effort as that was never my intent. We don't have to use this approach, but it my opinion it seems like it would be simpler.

EnricoBilla commented 1 year ago

@jrasm91 Yes of course I am absolutely happy to discuss the solution. And sorry for my previous message, but I was in a bad mood and I got overwhelmed.

If you guys think that one is the best approach I'm totally supporting the decision, also because maybe merging months of changes now requires even more effort than the actual PR. In the end I'm happy to have learnt new stuff about OAuth and OIDC since I've never worked with them πŸ˜„

jrasm91 commented 1 year ago

You did great for never having worked with it lol. And yes, this project seems to move so fast. I would not envy trying to get your conflicts resolved either.

I think most are on board with only using oauth to login via the access token and to keep using the immich jwt for all other API calls.

As far as two factor, I think it would make more sense to spend time working on other stuff in immich and leave that functionality to a dedicated auth service. It does bring up some good questions about possibly adding the ability to disable passwords altogether for immich. Or, for users that are using oauth.

I also noticed you did the mobile implementation. My PR doesn't have any mobile stuff in it, so if we go with it you could definitely help with that effort. Maybe even reuse what you already have.

Some of the implementation for this stuff ranges from trivial to really complicated, so I think incrementally adding new features might be the way to go. My PR is some basic functionally for logging in and the thought would be adding more complicated stuff and options afterwards.

I also saw there was a PR for moving some configuration stuff to the database and being able to configure it via the website. That would be a good candidate for all of the oauth options in the future as well.

EnricoBilla commented 1 year ago

@jrasm91 The mobile implementation was working, it would just require changing the endpoints for callback etc... It was working both for Android and iOS. The web integration was working but I feel the code is ugly since I've never worked with Svelte frontend 😬

For the configuration part through OAuth I do not get what do you mean

christiaangoossens commented 1 year ago

@EnricoBilla You did a good job on implementing this! Using OAuth2 tokens from another provider to authorize your own API requests just isn't the usual route, what @jrasm91 did is the more usual approach.

With regards to the possible failure scenario, idk if Immich has 2FA, but it is important to keep in mind that you do not want people to use the OIDC integration to bypass some authentication on an existing local account in any sense.

For the mobile part, do we even need specific code there? I guess the mobile app should just always open a webbrowser or web tab (do not use webviews for OAuth) and pass back the token, regardless of if it's OIDC or local.

EnricoBilla commented 1 year ago

@christiaangoossens Thanks! Let's go with @jrasm91's approach then!

Opening the web browser to retrieve the token would work I think, then it wouldn't be needed the specific code. That has still to be implemented in the webserver code

akoyaxd commented 1 year ago

Does it maybe make sense then to implement OAuth for local logins as well? Sounds like a good practice to no save the password in the app but just an access token, that can be revoked seperately.

EnricoBilla commented 1 year ago

Does it maybe make sense then to implement OAuth for local logins as well? Sounds like a good practice to no save the password in the app but just an access token, that can be revoked seperately.

I think it's best for apps to not handle authentication and authorization by themeselves. That's the reason I wanted to implement OAuth2/OIDC in Immich in the first place.

jrasm91 commented 1 year ago

Does it maybe make sense then to implement OAuth for local logins as well? Sounds like a good practice to no save the password in the app but just an access token, that can be revoked seperately.

I believe this is how immich already works. We store an immich jwt access token for future requests.

alextran1502 commented 1 year ago

@EnricoBilla Thank you for being a trailblazer in this PR. I apologize that this PR didn't get merge sooner. We now have the OIDC implementation from @jrasm91 with a more straightforward approach. Therefore, it will benefit us as maintainers.

I appreciate your time; this PR is one of the highest efforts we've ever had. Thank you very much.

Implemented in #884

EnricoBilla commented 1 year ago

@alextran1502 Thanks for your nice words and congrats to @jrasm91 for the awesome work! I'm happy to finally see OAuth implemented in ImmichπŸŽ‰