Add applied_permission_level from the model claim; remove unbacked JW… - #1168
Draft
mborodii-prog wants to merge 2 commits into
Draft
Add applied_permission_level from the model claim; remove unbacked JW…#1168mborodii-prog wants to merge 2 commits into
mborodii-prog wants to merge 2 commits into
Conversation
…T fallback Introduces GET /model/claim as the authoritative source for a model_id-addressed recipe's permission info: applied_group fills applied_permission_group and role fills the new applied_permission_level, both overriding an explicit caller-supplied value if it disagrees (with a warning logged), so a caller can't simply claim a higher role than the model's database actually grants them. run(model_id, variables={ "applied_permission_level": "admin"}) now correctly resolves to the real role (e.g. "viewer") instead of trusting the caller. The lookup is best-effort - a failure logs a warning and doesn't block the recipe from loading. Also removes get_applied_permission_group(), the JWT-claim-decoding fallback for non-model_id recipes: confirmed directly in the Keycloak repo that no protocol mapper has ever emitted that claim, so it always returned None against a real token and only appeared to work in its own test, which fabricated one. applied_permission_group for a non-model_id recipe is now purely whatever the caller explicitly passes.
Contributor
Author
|
PLS merge after release new api_core prod version |
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.
Add applied_permission_level from the model claim; remove unbacked JWT fallback
Problem
Two gaps in
applied_permission_group/permission-related recipe variables:applied_permission_group, and that variable was really only ever populated from/model/metadata'sapplied_permission_groupfield. When a model is triggered directly bymodel_id(from Python, or with an explicitvariables=override), there was nothing filling this in authoritatively - a caller could passvariables={"applied_permission_level": "admin"}and the recipe would simply believe it, even if the model's real claim says"viewer".get_applied_permission_group()(JWT-claim decoding, used as the fallback for non-model_id recipes) has never had a real backing. Confirmed directly in the Keycloak repo: the only custom protocol mapper there (GroupIdProtocolMapper) emitsgroup_ids, nothing else -applied_permission_grouphas never been a real token claim. Against a genuine access token this function always returnedNone; it only "worked" in its own unit test because that test fabricates a token containing the claim.Fix
New authoritative source:
GET /model/claimAlready merged and deployed on API-Core (
origin/main), returns:{ "model_id": "828a934d-af73-4c8a", "role": "admin", "organization_id": "team-id", "applied_group": "Dev (WrangleWorks)", "applied_group_type": "group", "applied_group_id": "team-id" }wrangles/data.py::model_claim(id)- new function, calls this endpoint.wrangles/auth.py- newextract_applied_permission_group_from_claim(claim)(readsapplied_group) andextract_applied_permission_level(claim)(readsrole).wrangles/recipe.py::_load_recipe()- for a model_id-addressed recipe, best-effort calls/model/claim(a failure logs a warning and does not block the recipe from loading) and, when it succeeds:applied_permission_group←applied_group, authoritative - overrides an explicitvariables={"applied_permission_group": ...}too.applied_permission_level←role, same authoritative treatment. This is the new variable.Removed:
get_applied_permission_group()Behavior change to flag explicitly: a non-model_id recipe that references
${applied_permission_group}without the caller ever passing it now raisesValueError: Variable ${applied_permission_group} was not found.instead of silently resolving toNone. Previously this was a silent no-op default; now it's simply undefined unless explicitly provided, consistent with every other recipe variable. If any recipe in the wild relies on the old silent-Nonebehavior, this would need a hardcoded default reinstated instead - flagging for review since it's a compatibility-relevant change, not just an internal cleanup.Tests
tests/test_data.py-model_claimadded to the existing 401/403parametrized tests, plus a new success test.
tests/recipes/test_variables.py:test_applied_permission_level_variable_from_model_claim, test_applied_permission_level_variable_claim_overrides_explicit(the exact scenario from the request - explicit"admin"vs. real"viewer"),test_model_claim_failure_does_not_block_recipe_load`.tests/recipes/test_recipes.py,tests/recipes/wrangles/test_main.py-added
model_claimmocks to the existing model_id-based tests so they don't attempt a real network call now that_load_recipe()calls it unconditionally for every model_id-addressed recipe.