-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
feat: Add OIDC_CLIENT_SECRET and other changes for v2 #4254
feat: Add OIDC_CLIENT_SECRET and other changes for v2 #4254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a thorough review, just a quick glance!
docs/docs/documentation/getting-started/installation/backend-config.md
Outdated
Show resolved
Hide resolved
Generally looks good to me. I'm not familiar enough with OIDC to speak on the implementation, but the code all makes sense to me. Happy to approve, but I think we should wait a bit to get some more eyes on it |
Its the same for me. Looking through the code everything does make sense to me, but i don't use any central auth provider, so i don't have anything setup to do some real testing. |
I've been running this on my personal instance (using Authelia) for about a week now and all has been smooth. I've also tested with other IdPs, namely Google and Authentik to verify compatibility. Really the only change from the old implementation is that everything is handled on the backend so all communication between Mealie and the IdP is done via back-channel and authlib handles the bulk of the work. Essentially the flow is as follows
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Let's get this in nightly for a bit so we can fix anything that comes up before release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @cmintey, thanks!
We'll get this on nightly and see if it causes problems with anyone's setup.
|
||
**Required** | ||
|
||
- After obtaining the **client secret** from your IdP, you must add it to Mealie using the `OIDC_CLIENT_SECRET` environment variable or via [docker secrets](../installation/backend-config.md#docker-secrets). This secret will not be logged on startup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This states that you can use a Docker secret for the OIDC_CLIENT_SECRET
, but that doesn't seem to work in practice. I don't see where OIDC_CLIENT_SECRET
is read from a file in this PR (which is how Docker secrets work). Am I missing something?
What type of PR is this?
What this PR does / why we need it:
Important
This PR introduces BREAKING CHANGES related to OIDC. Migration steps have been added to docs.
This PR implements point number 1 in the RFC: #4182. I decided against moving forward with point 2 and instead made it so that one or the other of
OIDC_USER_GROUP
orOIDC_ADMIN_GROUP
instead of the old behavior of needing to be in the user group if you wanted to be an admin. I will summarize the changes belowRequire a client secret
A new required environment variable was added called
OIDC_CLIENT_SECRET
. This is a breaking change!Adding this variable requires a re-work of the authentication scheme and so now all requests and auth processing happen server-side. An explanation of why this was necessary can be found in point 1 of the RFC linked above.
Change group requirements
Groups have always been optional and continue to be optional. The change is that for users in the
OIDC_ADMIN_GROUP
no longer also need to be inOIDC_USER_GROUP
. Existing setups will continue to work as expected, but this requirement has been dropped since it provided plenty of confusion to users.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
There is no real difference to the end-user, just one required and one optional migration step for server owners. Most of the authentication heavy lifting is now done by Authlib instead of Nuxt-Auth. This also makes the OAuth implementation more agnostic of a specific frontend framework or version.
Testing
New unit tests and existing E2E tests