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

Added possibility to set base identity provider for multiple chained … #621

Merged
merged 4 commits into from
May 9, 2023

Conversation

bogumillaska
Copy link

Adding the possibility to set base_identity_provider through tyk-operator.

Description

Currently you are able to set mutliple auth mechanism - however base_identity_provider remains empty which makes the dashboard confused about the true authentication type selected. That has implications for key creatation and update.
Until this PR - if you :

  1. create an apidefinition with use_basic_auth and use_mutual_tls_auth enabled
  2. process that through tyk-operator
  3. create policy with previously created API
  4. create key in the dashboard
  5. observe that the key only recognizes basic authentication and it is not enforcing mTLS certificate

Related Issue

NA

Motivation and Context

We are using multiple authentication on our project and we want our API to be fully compliant with standard to rule out unpredictable behavior

Test Coverage For This Change

Added unit test TestApiDefinitionBaseIdentityProviderWithMultipleAuthTypes in apidefinition_test.go following a similar structure to the mtls configuration.
Added config/samples/multiple-auth/httpbin_basic_authentication_and_mTLS.yaml
Executed manual tests on kind cluster using ce mode

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • Make sure you are updating CHANGELOG.md based on your changes.
  • My change requires a change to the documentation.
    • If you've changed APIs, describe what needs to be updated in the documentation.
  • I have updated the documentation accordingly.
  • If you've changed API models, please update CRDs.
    • make manifests
    • make helm
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Check your code additions will not fail linting checks:
    • gofmt -s -w .
    • go vet ./...
    • golangci-lint run

@bogumillaska bogumillaska requested a review from a team as a code owner April 30, 2023 21:41
@bogumillaska bogumillaska requested review from komalsukhani and removed request for a team April 30, 2023 21:41
@komalsukhani
Copy link
Collaborator

@bogumillaska Thank you for the contribution. I have reviewed the PR it looks great just need one change.

@bogumillaska
Copy link
Author

Ah! @komalsukhani - thank you! Nice catch.
I also fixed a line from my previous contribution. However, I did find similar situations in other tests and I did not attempt to make a change of them all in this contribution.

@bogumillaska bogumillaska force-pushed the base_identity_provider branch from 99cbab4 to 59d5be2 Compare May 3, 2023 21:08
@komalsukhani
Copy link
Collaborator

However, I did find similar situations in other tests and I did not attempt to make a change of them all in this contribution.
Ohh. I will raise an internal ticket to fix them.

I have approved the PR. It will be merged once QA tests it and will be available in patch release of Operator.
Thank you again.

@komalsukhani komalsukhani self-requested a review May 5, 2023 13:22
Copy link
Collaborator

@komalsukhani komalsukhani left a comment

Choose a reason for hiding this comment

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

I was trying this PR and demoing to my team. Got some suggestions. Can you please resolve them?

bogumillaska and others added 2 commits May 5, 2023 19:09
@bogumillaska
Copy link
Author

bogumillaska commented May 5, 2023

@komalsukhani I applied all the changes.

@komalsukhani
Copy link
Collaborator

@bogumillaska Looks perfect!! I will hand it over to QA to test.

@singhpr singhpr merged commit dd1d182 into TykTechnologies:master May 9, 2023
buger pushed a commit that referenced this pull request May 22, 2024
#621)

* Added possibility to set base identity provider for multiple chained authentication

* Update config/samples/multiple-auth/httpbin_basic_authentication_and_mTLS.yaml

Co-authored-by: Komal Sukhani <komaldsukhani@gmail.com>

* Adding validation of base_identity_provided_by and fixing the tests to use valid values

---------

Co-authored-by: Komal Sukhani <komaldsukhani@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants