Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redirect OIDC #1911

Merged
merged 9 commits into from
Feb 20, 2024
Merged

Fix redirect OIDC #1911

merged 9 commits into from
Feb 20, 2024

Conversation

Meierschlumpf
Copy link
Collaborator

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

@Meierschlumpf Meierschlumpf added the 🐛 Bug Something isn't working label Feb 18, 2024
@Meierschlumpf Meierschlumpf self-assigned this Feb 18, 2024
manuel-rw
manuel-rw previously approved these changes Feb 18, 2024
Copy link

github-actions bot commented Feb 18, 2024

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
Copy link
Contributor

catrielmuller commented Feb 18, 2024

@Meierschlumpf. I don't tested, beacause it's almost 6AM here, but looks like this part it's not working properly (#1909 (comment)).

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
Copy link
Collaborator Author

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
Copy link
Contributor

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 Meierschlumpf marked this pull request as draft February 18, 2024 09:13
@Meierschlumpf
Copy link
Collaborator Author

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

ajnart
ajnart previously approved these changes Feb 18, 2024
@Meierschlumpf Meierschlumpf dismissed stale reviews from ajnart and manuel-rw via 20aaeff February 18, 2024 09:16
manuel-rw
manuel-rw previously approved these changes Feb 18, 2024
src/utils/auth/oidc.ts Outdated Show resolved Hide resolved
@Meierschlumpf
Copy link
Collaborator Author

This functionallity should be tested before merging

@catrielmuller
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

Just gonna make another image that you can try

@Meierschlumpf
Copy link
Collaborator Author

It's ready again with the same tag

@catrielmuller
Copy link
Contributor

It's ready again with the same tag

Works Perfect, Thanks you very much.

@Meierschlumpf
Copy link
Collaborator Author

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

@Meierschlumpf Meierschlumpf marked this pull request as ready for review February 19, 2024 06:08
manuel-rw
manuel-rw previously approved these changes Feb 20, 2024
src/utils/auth/oidc.ts Outdated Show resolved Hide resolved
src/utils/auth/oidc.ts Outdated Show resolved Hide resolved
src/utils/auth/oidc.ts Outdated Show resolved Hide resolved
@Meierschlumpf Meierschlumpf merged commit 5cd940f into dev Feb 20, 2024
4 checks passed
@Meierschlumpf Meierschlumpf deleted the fix-redirect-oidc branch February 20, 2024 19:35
truecharts-admin referenced this pull request in truecharts/public Mar 16, 2024
…caf77d7 by renovate (#19304)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/ajnart/homarr](https://github.com/ajnart/homarr) | patch |
`0.15.0` -> `0.15.2` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>ajnart/homarr (ghcr.io/ajnart/homarr)</summary>

### [`v0.15.2`](https://github.com/ajnart/homarr/releases/tag/v0.15.2)

[Compare
Source](https://github.com/ajnart/homarr/compare/v0.15.1...v0.15.2)

> \[!NOTE]\
> We've been working actively on working torwards version 1.0 which will
include many improvements to performance, security and the overall look
& feel of Homarr. It will greatly overhaul the technical architecture of
Homarr. This work is done by volunteers. Please consider supporting our
work via donations at https://opencollective.com/homarr

#### v0.15.2: Hotfix, OMV 7 support

- Fixed an issue with the image where it would notify you to upgrade to
`0.15.1`, even though you were running `0.15.1`
-   Added support for OMV 7
-   Updated Crowdin translations

#### What's Changed

- chore: increase version by
[@&#8203;manuel-rw](https://github.com/manuel-rw) in
[https://github.com/ajnart/homarr/pull/1960](https://github.com/ajnart/homarr/pull/1960)
- feat: OMV 7 support by
[@&#8203;hillaliy](https://github.com/hillaliy) in
[https://github.com/ajnart/homarr/pull/1959](https://github.com/ajnart/homarr/pull/1959)
- feat: Apply translation automation from select option to multi-select…
by [@&#8203;Tagaishi](https://github.com/Tagaishi) in
[https://github.com/ajnart/homarr/pull/1963](https://github.com/ajnart/homarr/pull/1963)
- chore: new Crowdin updates by
[@&#8203;ajnart](https://github.com/ajnart) in
[https://github.com/ajnart/homarr/pull/1949](https://github.com/ajnart/homarr/pull/1949)
- core: increase version to 0.15.2 by
[@&#8203;manuel-rw](https://github.com/manuel-rw) in
[https://github.com/ajnart/homarr/pull/1967](https://github.com/ajnart/homarr/pull/1967)

**Full Changelog**:
ajnart/homarr@v0.15.1...v0.15.2

### [`v0.15.1`](https://github.com/ajnart/homarr/releases/tag/v0.15.1)

[Compare
Source](https://github.com/ajnart/homarr/compare/v0.15.0...v0.15.1)

> \[!NOTE]\
> We've been working actively on working torwards version 1.0 which will
include many improvements to performance, security and the overall look
& feel of Homarr. It will greatly overhaul the technical architecture of
Homarr. This work is done by volunteers. Please consider supporting our
work via donations at https://opencollective.com/homarr

#### Version 0.15.1: Fixes wih SSO, OMV integration and weekly weather
forecast

##### SSO fixes & improvements

- Added environment variable `AUTH_OIDC_SCOPE_OVERWRITE` to override the
OIDC scopes
-   Fixed redirection for OIDC logins
- Added the environment variable `AUTH_LDAP_SEARCH_SCOPE` to modify the
LDAP search scope between `base`, `one` or `sub`.
- Added debug information on the login page when authentication
providers are set incorrectly:

![310673082-a376bd01-e6bf-449b-93e8-f050da3fdef8](https://github.com/ajnart/homarr/assets/30572287/41c844b0-ce8b-43e4-80f8-526b20f2684b)

##### OMV widget

[@&#8203;hillaliy](https://github.com/hillaliy) has contributed a new
system health widget that integrates with
https://www.openmediavault.org/

![image](https://github.com/ajnart/homarr/assets/30572287/13ea31ef-85f3-401c-a2e6-ca89783cf3ce)

##### Weekly forecast

The weather widget can now display a weekly forecast:

![image](https://github.com/ajnart/homarr/assets/30572287/14fab53a-2caa-49be-8591-6f8b7afb9fb7)

##### Lithuanian and Estonian languange support

We have added Lithuanian and Estonian to Homarr. As always, our
community can translate Homarr into these languages:
https://crowdin.com/project/homarr

##### Improved torrent tile performance & ordering

Thanks to our contributors, the torrent widget now uses virtualization
to lower the required work on the client when rendering the list of
torrents. This results in more fluid scrolling and resizing of the
widget:

https://github.com/ajnart/homarr/assets/162878798/8a21eec2-2f6e-4b0b-8653-7cd730d7d697

Ordering columns is also now possible:

![](https://github.com/ajnart/homarr/assets/26098587/78e9636e-9ac5-44fe-aafc-d4df341ecf9b)

#### What's Changed

- feat: add environment variable to overwrite oidc scopes by
[@&#8203;Meierschlumpf](https://github.com/Meierschlumpf) in
[https://github.com/ajnart/homarr/pull/1913](https://github.com/ajnart/homarr/pull/1913)
- fix: redirect OIDC by
[@&#8203;Meierschlumpf](https://github.com/Meierschlumpf) in
[https://github.com/ajnart/homarr/pull/1911](https://github.com/ajnart/homarr/pull/1911)
- fix: set maximum size for indexer manager to 12 by
[@&#8203;Tagaishi](https://github.com/Tagaishi) in
[https://github.com/ajnart/homarr/pull/1912](https://github.com/ajnart/homarr/pull/1912)
- feat: add OMV integration / widget by
[@&#8203;hillaliy](https://github.com/hillaliy) in
[https://github.com/ajnart/homarr/pull/1879](https://github.com/ajnart/homarr/pull/1879)
- feat: add ldap search scope by
[@&#8203;Meierschlumpf](https://github.com/Meierschlumpf) in
[https://github.com/ajnart/homarr/pull/1948](https://github.com/ajnart/homarr/pull/1948)
- feat: AUTH_PROVIDER log when incorrect and show error in login page by
[@&#8203;Tagaishi](https://github.com/Tagaishi) in
[https://github.com/ajnart/homarr/pull/1943](https://github.com/ajnart/homarr/pull/1943)
- feat: add Lithuanian support by
[@&#8203;ajnart](https://github.com/ajnart) in
[https://github.com/ajnart/homarr/pull/1935](https://github.com/ajnart/homarr/pull/1935)
- feat: Mention Emby on Jellyfin integration by
[@&#8203;Tagaishi](https://github.com/Tagaishi) in
[https://github.com/ajnart/homarr/pull/1917](https://github.com/ajnart/homarr/pull/1917)
- feat: add weekly forecast to weather widget by
[@&#8203;hillaliy](https://github.com/hillaliy) in
[https://github.com/ajnart/homarr/pull/1932](https://github.com/ajnart/homarr/pull/1932)
- feat: add Estonian language by
[@&#8203;ajnart](https://github.com/ajnart) in
[https://github.com/ajnart/homarr/pull/1931](https://github.com/ajnart/homarr/pull/1931)
- chore: new Crowdin updates by
[@&#8203;ajnart](https://github.com/ajnart) in
[https://github.com/ajnart/homarr/pull/1890](https://github.com/ajnart/homarr/pull/1890)
- fix: death links in readme by
[@&#8203;Meierschlumpf](https://github.com/Meierschlumpf) in
[https://github.com/ajnart/homarr/pull/1953](https://github.com/ajnart/homarr/pull/1953)
- feat: Improve TorrentTile rendering performance by
[@&#8203;diederbert](https://github.com/diederbert) in
[https://github.com/ajnart/homarr/pull/1951](https://github.com/ajnart/homarr/pull/1951)
- fix: death app links by
[@&#8203;Meierschlumpf](https://github.com/Meierschlumpf) in
[https://github.com/ajnart/homarr/pull/1955](https://github.com/ajnart/homarr/pull/1955)
- feat: add column ordering in torrent widget by
[@&#8203;hillaliy](https://github.com/hillaliy) in
[https://github.com/ajnart/homarr/pull/1952](https://github.com/ajnart/homarr/pull/1952)

#### New Contributors

- [@&#8203;diederbert](https://github.com/diederbert) made their first
contribution in
[https://github.com/ajnart/homarr/pull/1951](https://github.com/ajnart/homarr/pull/1951)

**Full Changelog**:
ajnart/homarr@v0.15.0...v0.15.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNDkuMyIsInVwZGF0ZWRJblZlciI6IjM3LjI0OS4zIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
Status: ✅ Done
4 participants