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

Subdirectory support for OIDC and SocketIO #3665

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Nov 29, 2024

Brief summary

This is another PR in preparation for switching to subdirectory support in accordance with #3535

This covers two issues:

  1. SocketIO support for both new and existing clients
  2. Support for new and existing OIDC authentication setups

Which issue is fixed?

There are no existing bugs for this PR.

There should be no visible effect on existing (no-subdirectory) setups.

In-depth Description

SocketIO support

After the planned switch to subdirectory support, we will have both existing (no subdirectory) clients and new clients (either using a subdirectory or not). Since our SocketIO lives outside of the normal express middleware handlers, we need to support both subdirectory and no-subdirectory paths to access it.

This is achieved by bringing up two identical SocketIO servers that only differ in their paths. One uses the standard /socket.io path, and the other uses ${subdirectory}/socket.io (e.g. /audiobookshelf/socket.io). This way, legacy clients can still connect to the no-directory SocketIO server.

OIDC Auhentication support

OIDC authentication setups need to register a couple of authorized callback URLs in the OAuth provider. After the provider performs the authentication, it checks the callback URL it got from the server against the authorized callback URLs, and only calls back the server if it found a match.

So, in order for ABS servers to work after switching to subdirectory support, they need to remember which URLs were registered with the provider:

  • For existing OIDC setups, we use no-subdirectory URLs.
  • For new OIDC setups, we let the user choose between no-subdirectory and subdirectory URLs

This way, exising OIDC setups will continue to work without any manual setup required after the planned switch to subdirectory support.

To achieve this, we introduce a new server setting, authOpenIDSubfolderForRedirectURLs.
For existing OIDC setups it would be set to None (empty string). For new setups after the planned switch, it could be set to either None, or ${subdirectory} (e.g. /audiobookshelf). Authentication.vue was modified to enable setting it.

How have you tested this?

This was extensively tested using existing (no-subdirectory) and new (subdirectory) clients.

The server migration to add the new OIDC subfolder server setting was unit-tested as well as run on a test database with and without an existing OIDC setup.

@mikiher mikiher marked this pull request as ready for review November 29, 2024 03:50
@mikiher mikiher changed the title Subdirectory support for OIDC and WebSockets Subdirectory support for OIDC and SocketIO Nov 29, 2024
@advplyr
Copy link
Owner

advplyr commented Dec 2, 2024

I tested this with authentik OIDC and it is working well for both cases.

Do you think we should comment out the setting in authentication.vue until this is ready? I'm guessing it won't be long after release for someone to ask about that setting.

@mikiher
Copy link
Contributor Author

mikiher commented Dec 3, 2024

Yes, that's possible, although I had two purposes in mind with this setting:

  1. Setting the value (where currently only None is available)
  2. Making it explicit in the settings that the user needs to authorize the URLs in their IDP settings (this currently only appears in the last sentence of the documentation) I'm sure there are some users who want this feature but don't RTFM.

@advplyr
Copy link
Owner

advplyr commented Dec 3, 2024

It was helpful having the redirect URLs in the settings there.
Working great, thanks!

@advplyr advplyr merged commit 5fa0897 into advplyr:master Dec 3, 2024
6 checks passed
@advplyr
Copy link
Owner

advplyr commented Jan 11, 2025

Any idea about this #3823 (comment)?

@mikiher
Copy link
Contributor Author

mikiher commented Jan 12, 2025

I'll check this. I don't recall ever seeing this happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants