Repository navigation
feat(chart): set the OIDC provider and groups - #284
Merged
Merged
Conversation
Add the optional values auth.userInfoUri, auth.groups.* (claim, prefix, suffix, parentGroup, adminSubgroup) and auth.swaggerOauth.* (authorizationEndpoint, tokenEndpoint, scopes), so that deployments with another OIDC provider than ELIXIR AAI (e.g., LS Login) can set the user info endpoint, the group names and the Swagger UI's endpoints. Each is passed on to tesk-api only when set, so tesk-api's defaults apply otherwise; the schema checks that the endpoints are HTTP(S) URLs. auth.envBaseGroup and auth.envSubgroup are passed on only when set too, and as strings, so that the parent group can make up the environment group; the notes warn if auth.groups.parentGroup is set while auth.envBaseGroup replaces it. The chart no longer sets the Swagger UI's scopes itself (in a variable other than tesk-api's documented one), so tesk-api's default scopes, which are the same and follow the groups claim, apply unless auth.swaggerOauth.scopes is set. Document the values, an example for LS Login (endpoints checked against its discovery document) and the upgrade notes, and fix the comment that called tesk-api's default group model that of LS Login. Closes #281 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Render the provider and group values with tesk.secretValue, so that whole numbers from values files (which Helm reads as floats) aren't rendered in scientific notation. Test the notes with auth.groups null. Document that the LS Login environment group itself must be released, and that group names are URL-encoded in the claim. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
tesk.secretValue now also renders the provider and group values under auth, so its error for whole numbers too large to be exact names them too, instead of calling every value a secret. Test that path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Contributor
Reviewer's GuideConfigurationDefine the optional OIDC and group settings, validate their values, and render only set values as strings for tesk-api, including the related warnings and removal of the old Swagger scopes override.
DocumentationDocument the new provider and group settings, LS Login configuration, changed environment-group semantics, and upgrade impact.
TestsTest provider and group defaults, value rendering and validation, warnings, and the LS Login example documented in the deployment guide.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #284 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 422 422
=========================================
Hits 422 422 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
uniqueg
marked this pull request as ready for review
November 10, 2026 21:26
Contributor
Linked issuesChecked at commit 9cd39fa #281 feat(chart): set tesk-api's OIDC and group settings · 4 of 4 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="charts/tesk/templates/common/tesk-deployment.yaml" line_range="116" />
<code_context>
+ {{- $groups := default dict .Values.auth.groups }}
+ {{- $swaggerOauth := default dict .Values.auth.swaggerOauth }}
+ {{- range $variable, $value := dict
+ "SECURITY_OAUTH2_RESOURCE_USER_INFO_URI" .Values.auth.userInfoUri
+ "TESK_API_AUTHORISATION_GROUPS_CLAIM" $groups.claim
+ "TESK_API_AUTHORISATION_GROUP_PREFIX" $groups.prefix
</code_context>
<issue_to_address>
**Access tokens sent over plaintext**
When a deployment configures `auth.userInfoUri` with an `http://` URL, tesk-api sends each caller’s bearer token to the configured user-info endpoint to validate it, so a network attacker can intercept the token and use it to access the API as that caller.
Require HTTPS for `auth.userInfoUri`, allowing HTTP only for an explicitly constrained local-development case if needed.
</issue_to_address>
### Comment 2
<location path="charts/tesk/templates/common/tesk-deployment.yaml" line_range="129" />
<code_context>
+ "TESK_API_SWAGGER_OAUTH_SCOPES" $swaggerOauth.scopes }}
+ {{- if include "tesk.isSet" $value }}
+ - name: {{ $variable }}
+ value: {{ include "tesk.secretValue" $value }}
+ {{- end }}
+ {{- end }}
</code_context>
<issue_to_address>
**Requests fail during provider rollout**
When a Helm upgrade changes the provider or group settings while the Deployment is serving traffic, the Deployment serves old and new API pods concurrently, and requests routed to pods with different provider or group settings enforce different authorization rules. Callers therefore see intermittent authentication or authorization failures until the rollout finishes.
Use a rollout strategy that prevents old and new authorization configurations from serving traffic simultaneously, or require a compatible staged configuration during provider changes.
</issue_to_address>
### Comment 3
<location path="charts/tesk/templates/_helpers.tpl" line_range="358" />
<code_context>
*/}}
{{- define "tesk.secretValue" -}}
{{- if and (kindIs "float64" .) (eq (floor .) .) -}}
{{- if and (lt . 9007199254740992.0) (gt . -9007199254740992.0) -}}
{{- printf "%.0f" . | quote -}}
{{- else -}}
</code_context>
<issue_to_address>
**Exact numeric settings fail to render**
When an auth provider or group value is supplied as the YAML integer 9007199254740992 or -9007199254740992, `tesk.secretValue` rejects these exactly representable integers because its bound check is strict, so Helm rendering fails instead of preserving the value.
Make the `tesk.secretValue` bounds inclusive so these exactly representable integers render successfully.
Also at `charts/tesk/templates/common/tesk-deployment.yaml:129-130`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and a configured user-info endpoint and group mapping now determine which tokens and group memberships tesk-api accepts for authorization. A wrong mapping could grant unintended access while deployed; reverting restores the chart settings, but any data exposed during that time cannot be recovered.
Blocking findings: charts/tesk/templates/common/tesk-deployment.yaml:116, charts/tesk/templates/common/tesk-deployment.yaml:129, charts/tesk/templates/_helpers.tpl:358
tesk-api sends the access token of every request to the user info endpoint, so the notes warn if auth.userInfoUri is an http:// URL. Such URLs stay allowed, for providers reached only within a trusted network (e.g., in the cluster). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets deployments with an OIDC provider other than ELIXIR AAI (e.g., LS Login) set tesk-api's provider and group settings through values. All new values are optional and are passed on only when set, so tesk-api's defaults apply otherwise:
auth.userInfoUrisetsSECURITY_OAUTH2_RESOURCE_USER_INFO_URI.auth.groups.claim,prefix,suffix,parentGroupandadminSubgroupsetTESK_API_AUTHORISATION_GROUPS_CLAIM,…_GROUP_PREFIX,…_GROUP_SUFFIX,…_PARENT_GROUPand…_ADMIN_SUBGROUP.auth.swaggerOauth.authorizationEndpoint,tokenEndpointandscopessetTESK_API_SWAGGER_OAUTH_AUTHORIZATION_ENDPOINT,…_TOKEN_ENDPOINTand…_SCOPES.auth.envBaseGroupandauth.envSubgroupare now passed on only when set, and always as strings. Before, they were passed as empty text, or unquoted (a0broke the Deployment). This letsauth.groups.parentGroupandauth.envSubgroupmake up the environment group whenenvBaseGroupis"". The notes warn ifparentGroupis set whileenvBaseGroupreplaces it.TESK_API_SWAGGER-OAUTH_SCOPES, which is not tesk-api's documented variable, and with the same value as tesk-api's default. tesk-api's default scopes now apply unlessauth.swaggerOauth.scopesis set; they also followauth.groups.claim.tesk.secretValue), not in scientific notation.Docs:
envBaseGroup/envSubgroupsemantics.https://login.aai.lifescience-ri.eu/oidc/userinfo, authorize…/authorize, token…/token, claimeduperson_entitlement. The entitlement formaturn:geant:lifescience-ri.eu:group:<vo>:<group>#aai.lifescience-ri.eucomes from Perun's entitlement docs. A test renders the example from the docs.values.yaml, the schema,configuration.mdanddeployment.md: the defaults are ELIXIR AAI's.Decisions:
TESK_API_AUTHORISATION_DELIMITER) is not exposed, because tesk-api recognises teams by:whatever it is (as the issue's proposal leaves it out).TESK_API_SWAGGER_OAUTH_IMPLICITis not exposed either.tesk.extraEnv(the issue's alternative): it's out of scope here and can get its own issue if wanted.envSubgroup: CSC,envBaseGroup: ECP_CLN). The rendered Deployment with default values differs fromdevonly by the removed scopes variable and the order and quoting of the two group variables. Behaviour is the same, but the pod restarts on upgrade.docs/releases/0.3.0.md) have a Features bullet (feat(chart): set tesk-api's OIDC and group settings #281, feat(chart): set the OIDC provider and groups #284), and the intro mentions the new settings.Verification
Local runs with Helm v3.22.1, as pinned in CI:
helm lint charts/tesk --strict: passed.helm template tesk charts/tesk --namespace tesk: rendered.REQUIRE_HELM=1 pytest tests/test_chart.py: 478 passed (the dev branch has 425; 53 new). The new tests cover every value inauthandnoauthmode, null and empty values, numbers and booleans from--setand from values files, schema rejections, the NOTES warning, and the LS Login example from the docs.coverage run -m pytest --ignore=tests/test_chart.py: 156 passed, 100% coverage.pre-commit run --all-files: passed.mkdocs build --strict: passed.Chart install). The sandbox blocks the Docker socket and kind isn't installed, so CI runs it.CI (run 42904474370): all checks passed, including Chart install (kind), Chart, Lint, Test, Docs, Release readiness, CI passed, CodeQL, Codecov and the Read the Docs preview.
Review
Full review (Helm chart):
adversarial-reviewerandquality-reviewer, then a second adversarial round after the logic fix. All findings were fixed:auth.groups: nullwas added.PROVIDERandPROVIDER_VALUESare now checked to stay in sync.Declined review findings
EBI,GA4GH:GA4GH-CAP,ADMIN, the ELIXIR prefix and suffix) aren't tied to tesk-api by a test. They were checked by hand against tesk-api v1.2.0'sapplication.properties, and the chart pins tesk-api 1.2.0 as itsappVersion.Open questions
None.
Closes #281
🤖 Generated with Claude Code
https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Summary by Sourcery
Enable Helm deployments to configure OIDC providers and group authorization settings while preserving tesk-api defaults when values are unset.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: