Feedback Requested: Mealie v2 OAuth Implementation

[cmintey]

Earlier this year, Mealie gained support for authentication via OAuth using OpenIdConnect (OIDC). The implementation came with some confusions from the community surrounding the group permissions and the fact that a client secret was not required and all calls were done through the browser (thus causing issues with CSRF).

With Mealie v2 on the horizon, this brings the opportunity for breaking changes. I have some ideas for a v2 implementation of OAuth authentication that would simplify the setup and make it more in line with how other self-hosted applications implement it. This of course would come with a few breaking changes.

Breaking Changes

1. Require a client secret

This would mean that all requests and auth processing would happen server-side and would no longer be relying on the browser making requests. This would make it so CSRF is no longer an issue. Additionally, IdPs (like Google) which do not support the Authorization Code flow without a client_secret paramter would now work with Mealie.

2. Remove OIDC_USER_GROUP and OIDC_ADMIN_GROUP

These two configuration options end up causing a lot of confusing during the authentication process. With v2 introducing Households, I imagine this will become an even larger point of confusion.
I suggest we remove them and let the IdP handle access control. This would also remove the need for the groups scope. Of course this means that you could no longer configure a user as an admin via your IdP (though this can still be done manually within Mealie).

What benefits do these changes provide?

• Less confusion during setup
• More IdPs will be supported
• Less errors due to CSRF
• Better error transparency
• Cleaner implementation from a coding perspective

Potential Enhancements

Specify which group a user is added to in Mealie via the IdP

Some self-hosted applications have the idea of groups and can handle adding a user to a group during the authentication process. The way this is accomplished in Vikunja, for example, is by using a custom claim vikunja_groups which contains the group name and an identifier for creating groups. This idea could be utilized by Mealie and users could be automatically added to groups and/or households

Final Thoughts

Breaking changes are never fun, but are sometimes required for better outcomes. The above changes are minimal and should not be perceivable from an end-user perspective, but will hopefully reduce headaches during the initial setup. It would also make Mealie's implementation more similar to other self-hosted services.

I would like to hear the communities thoughts if this is a desirable change or if the current implementation is "good enough" and don't want to have such a breaking change.

[michael-genson]

Broadly these changes make sense to me. I also agree that now is a good time for breaking changes with V2 coming. I'm not a regular OIDC user, though, so I don't have much to say on the actual changes themselves.

[shiruken]

Regarding Point 2, Authelia's dev believes the current Mealie OIDC system would be the correct method of implementing the OpenID Connect specifications:

While there is merit in the idea of handling RBAC [role-based access control] in individual clients on the OpenID Connect Provider, it's rare to see OpenID Connect Relying Party's that do not handle it internally and that behavior in my opinion is strongly encouraged by the spec (i.e. not explicitly stated I don't think, but it seems implied).

Generally an RP [Relying Party] should request the appropriate scopes so it has access to claims that are relevant in determining if a user is authorized and what they are authorized for. For example we pass the groups claim when the groups scope is requested, which Gitea should be able to utilize to assign groups to the user.

While Authelia ultimately added support for RBAC for OIDC clients, it might be worth considering what's the most compliant implementation.

[cmintey]

Thanks for that tidbit. I’m certainly not opposed to leaving that feature in as it is currently optional (originally it wasn’t) and just hardening the docs surrounding the feature

[shiruken]

Makes sense. I won't pretend to have any knowledge about the OIDC spec, I just happened to run across that issue yesterday while troubleshooting a client that doesn't support the groups claim.

[boc-the-git]

I don't use or have any experience with OIDC, so I have nothing to really weigh in with, but just wanted to say I love the consultation @cmintey ❤️

[cromelex]

I think 1. is definitely a good thing to implement.