ajnart / homarr

Customizable browser's home page to interact with your homeserver's Docker containers (e.g. Sonarr/Radarr)
https://homarr.dev
MIT License
6.02k stars 275 forks source link

Fix redirect OIDC #1911

Closed Meierschlumpf closed 7 months ago

Meierschlumpf commented 7 months ago

This pull request fixes #1909

The redirect uri for oidc was sometimes wrong and so it was not able to eighter redirect to the correct page or to use the registered redirect url

github-actions[bot] commented 7 months ago

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 16.8% 4945 / 29421
🔵 Statements 16.8% 4945 / 29421
🔵 Functions 6.79% 32 / 471
🔵 Branches 38.01% 130 / 342
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/pages/api/auth/[...nextauth].ts 0% 0% 0% 0% 1-7
src/pages/auth/login.tsx 24.7% 75% 50% 24.7% 33-217, 235-241
src/server/auth.ts 20.64% 100% 0% 20.64% 24-125, 128-136, 144-155
src/utils/auth/index.ts 78.18% 100% 0% 78.18% 44-55
src/utils/auth/oidc.ts 49.39% 66.66% 50% 49.39% 40-81
Generated in workflow #6352
catrielmuller commented 7 months ago

@Meierschlumpf. I don't tested, beacause it's almost 6AM here, but looks like this part it's not working properly (https://github.com/ajnart/homarr/issues/1909#issuecomment-1951014654).

A HotFix can be accept an env var to override this parameter here:

https://github.com/ajnart/homarr/blob/fix-redirect-oidc/src/utils/auth/oidc.ts#L23C3-L23C16

 authorization: { 
    params: { 
        redirect_uri: env.AUTH_OIDC_REDIRECT_URI,
        scope: 'openid email profile groups' 
    }
 },
Meierschlumpf commented 7 months ago

Hey @catrielmuller thanks for your input. I think this would be a good alternative option if the current one would not work as intended. The issue with the additional env variable lies in the fact that you may have multiple redirect_url's (for example an internal and external) and so I suggest that we first try it with the current solution and if that does not work we can of course move forward with your solution. Thanks ❤️

catrielmuller commented 7 months ago

The call for https://subdomain.example.com/api/auth/signin/oidc it's working fine and the cookie it's set correctly. However on the redirect to the OIDC the payload keeps as localhost redirect_uri: http://localhost:7575/api/auth/callback/oidc

Meierschlumpf commented 7 months ago

Okay I'm gonna make a fix for that. I would guess that once this option is defined we also do no longer need the redirect callback

Meierschlumpf commented 7 months ago

This functionallity should be tested before merging

catrielmuller commented 7 months ago

@Meierschlumpf , I tested this and now the redirect_uri it's working fine. However when the redirect to: https://www.example.com/api/auth/callback/oidc?code=cXXXXXXXXXX&state=XXXXX Produce a 302 to http://localhost:7575 and should be to https://www.example.com/ image If I enter again to https://www.example.com/ the session it's working perfectly I checked the code and the easy way to test is try to access to:

https://www.example.comapi/api/auth/callback/oidc (Without any parameters) and that should be redirect to https://www.example.comapi/api/auth/error?error=OAuthCallback However redirects to http://localhost:7575/api/auth/error?error=OAuthCallback

catrielmuller commented 7 months ago

Btw, we should update the documentation and explain that the implementation use RS256 to read the JWT, and HS256 it's not supported. So each OAuth provider should have this configuration on their options.

Meierschlumpf commented 7 months ago

Okay, thought NextAuth would be smart enough, i guess I'm gonna add the redirect callback then. Thanks a lot for testing. Gonna do that in about an hour

catrielmuller commented 7 months ago

Okay, thought NextAuth would be smart enough, i guess I'm gonna add the redirect callback then. Thanks a lot for testing. Gonna do that in about an hour

They are complete focused on migrate to Auth.JS 🤦

Meierschlumpf commented 7 months ago

Just gonna make another image that you can try

Meierschlumpf commented 7 months ago

It's ready again with the same tag

catrielmuller commented 7 months ago

It's ready again with the same tag

Works Perfect, Thanks you very much.

Meierschlumpf commented 7 months ago

Thank you a lot for testing and sharing your thoughts, really appreciated them. Gonna check that for the docs tonight (11 hours) and merge the pull request asap. Then we'll release 0.15.1