SK-3039:Fix Skyflow clients bearertokens roles & context. - #273
Merged
Conversation
…scope SK-3039:Fix missing roles & ctx in bearerToken.
yaswanth-pula-skyflow
requested review from
Devesh-Skyflow,
saileshwar-skyflow and
skyflow-bharti
August 4, 2026 11:29
skyflow-bharti
approved these changes
Aug 4, 2026
saileshwar-skyflow
approved these changes
Aug 4, 2026
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.
SK-3039: Fix missing
roles/ctxin bearer tokens and harden request/config input validationProblem
rolesandcontextset undercredentialsinadd_vault_config()never reached the generated bearer token.VaultClient.get_bearer_token()readroles/ctxfrom the top-level config, but both are only accepted nested undercredentials— top-level keys are rejected byvalidate_keys, so the lookup always resolved toNone: no error, just a token with noctxclaim and norole:scope, on first generation and every auto-refresh.contextwas validated asstronly, rejecting a dict/JSON object atbuild()even though the token-generation engine already supports it.SkyflowError: dicttokenson insert, bare-string items in detokenizedata, non-string elements inroles, non-listidson delete, and non-stringskyflow_id/table/column_name/file_nameon update and file-upload.validate_update_connection_configdroppedconnection_idfromvalidate_credentials, so credential errors on the update path lost config context that the add path already included.Changes
Bearer token
roles/ctxVaultClient.get_bearer_token()now builds token options from the resolvedcredentialsdict instead of the top-level config, and calls the sharedvalidate_token_optionsdirectly — so a directly-constructedVaultClientcan no longer skip roles/context validation. Keys are omitted when unset rather than passed asNone.pathandcredentials_string, config-level and common credentials, and connection configs.credentials.contextnow accepts astr,bool,int,float&dict. Dict keys are validated at config time (^[a-zA-Z0-9_]+$), so an invalid key fails atbuild()instead of on the first API call. Empty dict is rejected as an empty context, matching existing empty-string behaviour.empty/invaliderror messages forroles: a non-list now reports "Specify roles as an array" instead of "Specify at least one role".roles: []now raisesEMPTY_ROLESinstead of silently producing an unscoped token. Each element ofrolesmust now be a non-empty string, or validation raises — previously a non-string role was silently stringified into the OAuth scope.validate_update_connection_confignow passesconnection_idthrough tovalidate_credentials, so its error messages include the connection id like the add path already does.Insert / detokenize type safety
InsertRequest(tokens=...)with a non-list/non-dict-of-dicts value now raisesINVALID_TYPE_OF_DATA_IN_INSERTinstead of crashing in the diagnostic logging loop.DetokenizeRequest(data=...)with bare strings now always raisesINVALID_TOKENS_LIST_VALUE, instead of only crashing when a string happened to contain"token"as a substring.Delete / update / file-upload validation
DeleteRequest(ids=...)with a non-list value now raisesINVALID_IDS_TYPE.UpdateRequestandFileUploadRequestnow type-checkskyflow_id(newINVALID_SKYFLOW_ID_TYPEmessage), andFileUploadRequesttype-checkstable,column_name, andfile_name, raising the existing corresponding error instead of crashing on.strip().Behaviour change
roles: []and non-stringroleselements previously passed validation and produced an unscoped or corrupted token; both now raise. Everything else is either a crash fix or a message correction — the accept/reject boundary for already-valid input is unchanged.rolesandcontextremain optional; validation only runs when the key is present.Tests
tests/vault/client/test__client.py: string and dict context plus roles forwarded on the path and credentials-string flows; options omitted when unset; top-level config keys explicitly ignored;token_uricoexistence; refresh path verified directly and end-to-end throughinitialize_client_configuration().tests/utils/validations/test__validations.py: string/dict context accepted, empty dict, invalid ctx key, invalid types, config-scoped message variant, end-to-endvalidate_vault_config, the threerolescases plus non-string role elements, dicttokenson insert, bare-string detokenizedata, non-list deleteids, non-stringskyflow_id/table/column_name/file_nameon update and file-upload, andconnection_idincluded invalidate_update_connection_configerror messages.tests/client/test_skyflow.py: end-to-endbuilder().add_vault_config(...).build()asserting the options handed to the token engine.roles/ctxfrom the client test fixture — that shape is rejected by real validation.