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

Allow configuration of OIDC scopes to request to support role-claim #23

Closed
strazto opened this issue Apr 28, 2022 · 8 comments · Fixed by #24
Closed

Allow configuration of OIDC scopes to request to support role-claim #23

strazto opened this issue Apr 28, 2022 · 8 comments · Fixed by #24
Labels
enhancement New feature or request

Comments

@strazto
Copy link
Collaborator

strazto commented Apr 28, 2022

Is your feature request related to a problem? Please describe.

I recently decided to try to configure role-based access. I used the following configuration:

---
authelia:
  OidEndpoint: https://authelia.example.com/.well-known/openid-configuration/
  OidClientId: jellyfin
  OidSecret: <omitted>
  Enabled: true
  EnableAuthorization: true
  EnableAllFolders: true
  EnabledFolders: []
  Roles: ["jellyfin_user"]
  AdminRoles: ["jellyfin_admin"]
  EnableFolderRoles: false
  FolderRoleMapping: []
  RoleClaim: groups

Fields of interest are here:

authelia:
  Roles: ["jellyfin_user"]
  AdminRoles: ["jellyfin_admin"]
  RoleClaim: groups

However, I found the response was "Error. Check Permissions", with the following log entry:

[12:57:50] [WRN] [7] Jellyfin.Plugin.SSO_Auth.Api.SSOController: OpenID user su has one or more incorrect role claims: [{"Type": "jti", "Value": "***redacted***"}, {"Type": "name", "Value": "Superuser"}, {"Type": "preferred_username", "Value": "su"}, {"Type": "rat", "Value": "1651114669"}, {"Type": "sub", "Value": "su"}]. Expected any one of: ["jellyfin_user"]

Per the authelia oidc config spec: https://www.authelia.com/docs/configuration/identity-providers/oidc.html#groups , I learned that the "groups" attribute requires the client to request the "groups" scope.

To test this, I modified the request to include this scope:

https://github.com/9p4/jellyfin-plugin-sso/compare/main...matthewstrasiotto:authelia_groups?expand=1

diff --git a/SSO-Auth/Api/SSOController.cs b/SSO-Auth/Api/SSOController.cs
index ad60764..f24e44e 100644
--- a/SSO-Auth/Api/SSOController.cs
+++ b/SSO-Auth/Api/SSOController.cs
@@ -76,7 +76,7 @@ public SSOController(ILogger<SSOController> logger, ISessionManager sessionManag
                 ClientId = config.OidClientId,
                 ClientSecret = config.OidSecret,
                 RedirectUri = GetRequestBase() + "/sso/OID/r/" + provider,
-                Scope = "openid profile",
+                Scope = "openid profile groups",
             };
             options.Policy.Discovery.ValidateEndpoints = false; // For Google and other providers with different endpoints
             var oidcClient = new OidcClient(options);
@@ -243,7 +243,7 @@ public async Task<ActionResult> OidChallenge(string provider)
                 ClientId = config.OidClientId,
                 ClientSecret = config.OidSecret,
                 RedirectUri = GetRequestBase() + "/sso/OID/r/" + provider,
-                Scope = "openid profile"
+                Scope = "openid profile groups"
             };
             options.Policy.Discovery.ValidateEndpoints = false; // For Google and other providers with different endpoints
             var oidcClient = new OidcClient(options);

And indeed, requesting "groups" correctly confers user roles as well as admin roles.
It appears that for some OIDC providers, (Authelia, at the very least), additional scopes may need to be requested.

Describe the solution you'd like

Rather than hard-code these scopes into the request as I did to test the matter, I propose adding an additional config option, "additional oidc scopes" (or something) that allows the user to specify additional scopes to include in the request.

Describe alternatives you've considered

Hard-code this in, allow more opinionated provider presets (Preset, keycloak, preset authelia, etc), for my own use-case I could probably have supplied a list of usernames i wanted to get admin, but this defeats the purpose of membership based role checking. (All of these ideas are bad)

@strazto strazto added the enhancement New feature or request label Apr 28, 2022
@strazto
Copy link
Collaborator Author

strazto commented Apr 28, 2022

I'm happy enough to implement this change, though I guess outstanding questions for @9p4 are:

  • What's your preferred data-type for OidScopes?
    • Personally, I'd be happy enough using space separated strings, but I'd also be fine with an array of strings.

In terms of work required, this change should be rather easy:

  • Add OidScopes to OidConfig class:
    /// <summary>
    /// Gets or sets the claim to check roles against. Separated by "."s.
    /// </summary>
    public string RoleClaim { get; set; }
    /// <summary>
    /// Gets or sets the default provider the user after logging in with SSO.
    /// </summary>
    public string DefaultProvider { get; set; }
    }
  • Append config.OidScopes to scope param in request
    var options = new OidcClientOptions
    {
    Authority = config.OidEndpoint,
    ClientId = config.OidClientId,
    ClientSecret = config.OidSecret,
    RedirectUri = GetRequestBase() + "/sso/OID/r/" + provider,
    Scope = "openid profile",
    };
  • Add "Additonal OID Scopes" to config page

@strazto
Copy link
Collaborator Author

strazto commented Apr 28, 2022

By the way, assuming this issue is considered, the following would consitute a complete & valid authelia config:

---
authelia:
  OidEndpoint: https://authelia.example.com/.well-known/openid-configuration/
  OidClientId: jellyfin
  OidSecret: <omitted>
  Enabled: true
  EnableAuthorization: true
  EnableAllFolders: true
  EnabledFolders: []
  # No need to request openid + profile
  OidScopes: [ "groups" ]
  Roles: ["jellyfin_user"]
  AdminRoles: ["jellyfin_admin"]
  EnableFolderRoles: false
  FolderRoleMapping: []
  RoleClaim: groups

@9p4
Copy link
Owner

9p4 commented Apr 28, 2022

Personally, I'd be happy enough using space separated strings, but I'd also be fine with an array of strings.

I believe an array of strings would be best

@strazto

This comment was marked as off-topic.

@fservida
Copy link

Popped up during test of this new release, if the value in the plugin config is empty (empty empty, not empty JSON array), the request fails processing due to "Value cannot be null" error.
There might be the need for a sensible default for optional values.

@9p4
Copy link
Owner

9p4 commented Apr 30, 2022

That's intentional. The configuration is meant to be as explicit as possible when done through the API.

@fservida
Copy link

Sorry, was not clear, I'm talking mainly about the newly introduced web interface for OIDC, which does not validate user input and allows for empty boxes, probably worth it's own issue to track that; I just discovered that because this new field was added and was thus missing from my configuration.
I completely understand that if using directly the API all should be filled.

@9p4 9p4 mentioned this issue Apr 30, 2022
@9p4
Copy link
Owner

9p4 commented Apr 30, 2022

Tracking input validation on the admin page here

#2 (comment)

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

Successfully merging a pull request may close this issue.

3 participants