Skip to content
This repository has been archived by the owner on Jul 4, 2024. It is now read-only.

user_context header encode/decode processing #2735

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

PetarTodorovv
Copy link
Collaborator

@PetarTodorovv PetarTodorovv commented Nov 14, 2022

Description
In the hydrator component and more specifically in the consumre_context_provider we could receive non-ascii characters in the user_context header. They will be escaped/encoded and we need to unescape/decode before using the data.

Changes proposed in this pull request:

  • Unescape the user_context header before data processing
  • adapt unit and e2e tests

Pull Request status

  • Implementation
  • Unit tests
  • Integration tests
  • chart/compass/values.yaml is updated
  • [N/A] Mocks are regenerated, using the automated script

@PetarTodorovv PetarTodorovv added 👋 request review Review required do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 14, 2022
@PetarTodorovv PetarTodorovv requested a review from a team as a code owner November 14, 2022 07:16
@PetarTodorovv PetarTodorovv self-assigned this Nov 14, 2022
@PetarTodorovv PetarTodorovv requested review from a team as code owners November 14, 2022 07:16
@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 🦖 team-raptor Team Raptor Label labels Nov 14, 2022
@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 15, 2022
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 16, 2022
@kyma-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nyordanoff

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kyma-bot kyma-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2022
@PetarTodorovv PetarTodorovv removed 👋 request review Review required do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 16, 2022
@kyma-bot kyma-bot merged commit 5bc71b8 into main Nov 16, 2022
@PetarTodorovv PetarTodorovv deleted the user-context-encoded-header branch November 16, 2022 14:45
@@ -96,12 +107,30 @@ func (c *consumerContextProvider) Match(_ context.Context, data oathkeeper.ReqDa
}

func (c *consumerContextProvider) getUserContextData(userContextHeader string) (*userContextData, error) {
clientID := gjson.Get(userContextHeader, c.consumerClaimsKeysConfig.ClientIDKey)
decodedUserContextHeader, err := url.QueryUnescape(userContextHeader)
if err != nil {
Copy link
Member

@DimitarPetrov DimitarPetrov Nov 17, 2022

Choose a reason for hiding this comment

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

There is a fair amount of code duplication.

The same can be achieved with

if decodedUserContextHeader, err := url.QueryUnescape(userContextHeader); err == nil {
    userContextHeader = decodedUserContextHeader
}

And everything below stays the same.

Valid for the above method as well. Could be a one-liner.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 🦖 team-raptor Team Raptor Label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants