Skip to content

Added an optional OAUTH_ALLOWED_ROLES environment variable#1463

Open
JimKnoxx wants to merge 3 commits intogetfider:mainfrom
JimKnoxx:oauth-allowed-roles
Open

Added an optional OAUTH_ALLOWED_ROLES environment variable#1463
JimKnoxx wants to merge 3 commits intogetfider:mainfrom
JimKnoxx:oauth-allowed-roles

Conversation

@JimKnoxx
Copy link
Contributor

@JimKnoxx JimKnoxx commented Feb 25, 2026

  • Setting this prevents users without the specified roles from accessing the Fider instance

We use Fider in a private instance with OAUTH as only login method.
We only want some users (teachers and admins) to access the instance.
At the moment we can only use "obscurity" measurements to prevent students from accessing.

In this PR I (and Claude), added the OAUTH_ALLOWED_ROLES .env variable, a way to filter out the roles from the oauth json response of a user and perform access checks based on the roles that the user has.

Issue: #1464

Generated with Claude Code

- Setting this prevents users without the specified roles from accessing the Fider instance

Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
@JimKnoxx JimKnoxx force-pushed the oauth-allowed-roles branch from 5d971d7 to 015c03d Compare February 25, 2026 15:10
@JimKnoxx JimKnoxx changed the title Added the optional oauth allowed roles environment variable Added an optional OAUTH:allowed roles environment variable Feb 25, 2026
@JimKnoxx JimKnoxx changed the title Added an optional OAUTH:allowed roles environment variable Added an optional OAUTH_ALLOWED_ROLES environment variable Feb 25, 2026
@mattwoberts
Copy link
Contributor

Review: Design concern — global OAUTH_ALLOWED_ROLES vs per-provider JSONUserRolesPath

OAUTH_ALLOWED_ROLES is a single global env var, but JSONUserRolesPath is configured per OAuth provider. This creates a problem in multi-provider setups:

  • If you have Google OAuth (no roles path configured) + custom OAuth (with roles), Google users will always be denied when OAUTH_ALLOWED_ROLES is set, because hasAllowedRole() receives an empty roles slice and returns false.
  • Different providers may use different role names, but there's no way to configure allowed roles per provider.

Consider either:

  1. Making OAUTH_ALLOWED_ROLES a per-provider setting (stored alongside JSONUserRolesPath in the DB), or
  2. Skipping the role check when the provider has no JSONUserRolesPath configured (i.e., only enforce when the provider actually returns roles)

Option 2 is simpler and would only require checking whether the provider's JSONUserRolesPath is non-empty before calling hasAllowedRole().


Additional notes

  • Missing test coverage for hasAllowedRole()roles_test.go tests extractRolesFromJSON but not the actual access-control gate function. This should have tests covering: empty config (allow all), matching role, no matching role, empty user roles with config set.

  • Separator conventionOAUTH_ALLOWED_ROLES uses ; as separator, which is unusual for env vars. Consider using , for consistency. The role string splitting in extractRolesFromJSON tries ; then , which adds ambiguity.

  • navigateJSONPath duplicates app/pkg/jsonq — The dot-notation path traversal in navigateJSONPath() reimplements what jsonq.Query.get() already does (app/pkg/jsonq/jsonq.go:89). The array extraction logic (roles[].id) is genuinely new, but the path navigation portion could reuse jsonq to avoid duplication.

  • locale/de/client.json has 66 additions / 63 deletions that appear to be unrelated reformatting — consider splitting that into a separate commit.

@mattwoberts
Copy link
Contributor

@JimKnoxx Hi there - there's a couple of things there to look at - the jsonq thing i'd deffo look at for code duplication. If you do need any additional json stuff that might also be reusable then adding it to that package might be a good move too - rather than keeping it with the oauth stuff?

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