Skip to content

feat: add a way to check for specific OIDC claims #11603

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.

Sign up for GitHub

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

Jump to bottom
Merged
kobergj merged 10 commits into from
Sep 24, 2025
Merged

feat: add a way to check for specific OIDC claims #11603

kobergj merged 10 commits into from
Sep 24, 2025

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Aug 13, 2025 •
edited by kobergj
Loading

Feature Updated

Feature updated as we current guarantee secure access with the old approach. We are using users and groups endpoint also for regular users. Only if you want to list all users you need 2fa. The new approach is as follows:

Proxy middleware evaluates the context and adds a header to the request. If mfa is disabled the middleware will always set the header to true. If mfa is enabled the middleware compares the acr value with the configured values and sets header to true if one level matches. This way services do not need to care if mfa is enabled or disabled - they always assume it is enabled.

Graph service reads the header and adjusts context. The handlers check the context when they enter sensitive parts of the code. This way we have full control which parts of the code we want to mfa protect.

We introduced a new mfa package so future services (e.g. ocdav service) can reuse the package when they introduce 2fa into their stack.

(original comment hidden for readability)

@jvillafanez jvillafanez self-assigned this Aug 13, 2025
@update-docs
Copy link

update-docs bot commented Aug 13, 2025

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

mmattel
mmattel reviewed Aug 14, 2025
View reviewed changes
@mmattel mmattel mentioned this pull request Aug 14, 2025
Closed
@jvillafanez jvillafanez marked this pull request as ready for review August 18, 2025 11:10
@LukasHirt LukasHirt mentioned this pull request Aug 18, 2025
Closed
5 tasks
mklos-kw
mklos-kw previously approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@mklos-kw mklos-kw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with: owncloud/web#12942
the guarded ocis endpoints returned 200 after OTP instead of 401.

@jvillafanez jvillafanez mentioned this pull request Aug 20, 2025
Merged
7 tasks
2403905
2403905 previously approved these changes Aug 21, 2025
View reviewed changes
@kobergj kobergj force-pushed the oidc_claims_checker branch from 878a383 to 2bf68d0 Compare September 11, 2025 12:34
@kobergj
Copy link
Collaborator

kobergj commented Sep 11, 2025

just rebased to master - no conflicts - no re-review needed

@kobergj kobergj marked this pull request as draft September 11, 2025 12:38
@kobergj kobergj force-pushed the oidc_claims_checker branch from f3d38c9 to ef7be90 Compare September 12, 2025 13:38
kobergj
kobergj previously requested changes Sep 12, 2025
View reviewed changes
@kobergj kobergj force-pushed the oidc_claims_checker branch from ef7be90 to 9e37dde Compare September 12, 2025 13:47
kobergj
kobergj reviewed Sep 12, 2025
View reviewed changes
jvillafanez and others added 7 commits September 17, 2025 12:42
@jvillafanez
feat: add a way to check for specific OIDC claims
ed95f21 
For now, only some hardcoded paths will check the claims (if there is a
checker configured). If the check fails for those paths, the
authentication will fail and it will return a 401 error
@jvillafanez
fix: CI complains
789ac03
@jvillafanez
feat: allow custom headers on authentication failures
c0e5c16
@jvillafanez
fix: adjust unit tests
7b053e8
@jvillafanez
docs: add missing doc information
b738f82
feat: use middleware for mfa checks
5052c65 
Signed-off-by: Julian Koberg <[email protected]>
feat: move mfa checks to handlers
eeeeba1 
Signed-off-by: Julian Koberg <[email protected]>
@kobergj kobergj force-pushed the oidc_claims_checker branch 2 times, most recently from 9b3e17c to a8d744b Compare September 17, 2025 12:11
@kobergj kobergj force-pushed the oidc_claims_checker branch 4 times, most recently from 8b272a3 to 6fb682d Compare September 17, 2025 13:12
mmattel
mmattel reviewed Sep 17, 2025
View reviewed changes
mmattel
mmattel reviewed Sep 17, 2025
View reviewed changes
feat: clean up mfa middleware
824a71d 
Signed-off-by: Julian Koberg <[email protected]>
@kobergj kobergj force-pushed the oidc_claims_checker branch from 6fb682d to 824a71d Compare September 17, 2025 13:56
mmattel
mmattel reviewed Sep 17, 2025
View reviewed changes
@kobergj kobergj marked this pull request as ready for review September 17, 2025 14:50
@kobergj kobergj dismissed their stale review September 17, 2025 14:51

cant review my own changes

jvillafanez
jvillafanez commented Sep 17, 2025
View reviewed changes
@jvillafanez
Copy link
Member Author

jvillafanez commented Sep 17, 2025

For the mfa.go file, I think it's better to split the functions: on the one hand we have convenient "get & set" functions to deal with the data in the context, and on the other hand we have some functions dealing with HTTP headers.
Just seeing the code, I'm confused. Are the HTTP functions "the main dish", while the context ones should be secondary, or on the contrary, should we focus on the context functions and only use the HTTP function if they fit?.
Including code documentation about the expected usage would also help.

Side note, I think we need to test what happens if the AuthLevelNames we configure in oCIS are missing in Keycloak. You might set different names in Keycloak and forget to change the default configuration in oCIS (you might get the same effect by setting a missing name in oCIS).
I don't expect oCIS to work, but we shouldn't leave the user stuck in a weird place or show a meaningless error.

@kobergj
Copy link
Collaborator

kobergj commented Sep 18, 2025 •
edited
Loading

For the mfa.go file, I think it's better to split the functions .. Are the HTTP functions "the main dish"

I can do that. the context functions where originally in revas ctx package. I moved them for simplicity. Both of them are the main dish. I wanted an encapsulated package that one could use when introducing 2fa into another service. No other imports needed. But if you prefer to have the functionality split we can do so. Will make it more difficult to add 2fa to other services though...

Side note, I think we need to test what happens if the AuthLevelNames we configure in oCIS are missing in Keycloak

Already did. This results in 403 as expected. We can not catch such errors because we can't identify such a configuration. We do not check which auth levels are configured in keycloak. Misconfiguration breaks your system.

mmattel
mmattel reviewed Sep 18, 2025
View reviewed changes
@kobergj kobergj force-pushed the oidc_claims_checker branch from f04c1bf to e984e02 Compare September 18, 2025 14:17
feat: document mfa package
dda6104 
Signed-off-by: Julian Koberg <[email protected]>
@kobergj kobergj force-pushed the oidc_claims_checker branch from e984e02 to dda6104 Compare September 18, 2025 14:23
@kobergj kobergj force-pushed the oidc_claims_checker branch from 2658192 to 04b829e Compare September 23, 2025 12:03
@kobergj
Copy link
Collaborator

kobergj commented Sep 23, 2025

@jvillafanez I think I catched all your points. Please review again 🙏

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 23, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
46.6% Coverage on New Code
10.2% Duplication on New Code

See analysis details on SonarQube Cloud

@jvillafanez
Copy link
Member Author

jvillafanez commented Sep 24, 2025 •
edited
Loading

I guess that, as the owner of the PR, I can't approve the changes myself (github doesn't let me). Code looks good, so it's good to merge to me 👍

@kobergj kobergj merged commit 2f1ab88 into master Sep 24, 2025
4 checks passed
@kobergj kobergj deleted the oidc_claims_checker branch September 24, 2025 11:43
ownclouders pushed a commit that referenced this pull request Sep 24, 2025
@kobergj
Merge pull request #11603 from owncloud/oidc_claims_checker
d72e602 
feat: add a way to check for specific OIDC claims
ownclouders pushed a commit that referenced this pull request Sep 25, 2025
@kobergj
Merge pull request #11603 from owncloud/oidc_claims_checker
ba6d000 
feat: add a way to check for specific OIDC claims
ownclouders pushed a commit that referenced this pull request Sep 25, 2025
@kobergj
Merge pull request #11603 from owncloud/oidc_claims_checker
7204a9e 
feat: add a way to check for specific OIDC claims
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mmattel mmattel mmattel left review comments

@kobergj kobergj kobergj left review comments

@LukasHirt LukasHirt LukasHirt left review comments

@2403905 2403905 2403905 approved these changes

@mklos-kw mklos-kw mklos-kw approved these changes

Assignees

@jvillafanez jvillafanez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jvillafanez @kobergj @mmattel @2403905 @LukasHirt @mklos-kw