Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

special case for basic auth added #2018

Merged
merged 3 commits into from
Dec 24, 2018
Merged

special case for basic auth added #2018

merged 3 commits into from
Dec 24, 2018

Conversation

dencoded
Copy link
Contributor

@dencoded dencoded commented Dec 7, 2018

Fix https://github.com/TykTechnologies/tyk-analytics/issues/976 #2038

Added new attributes org_id and username (boolean) attributes, which should be passed together when you query for username.

@dencoded dencoded requested a review from buger December 7, 2018 04:28
@buger
Copy link
Member

buger commented Dec 7, 2018

I get the idea, but I'm afraid this will not work. When dashboard sends the request to gateway API, it does not set the right API id.

@dencoded
Copy link
Contributor Author

dencoded commented Dec 7, 2018

it worked for me in dashboard - tried to find two different BA-keys (created for different BA apis).

the problem here that our gateway logic uses API-id just to get session storage, then it can find any key as they are prefixed with "apikey-" (no API id in prefix). I think dashboard picks latest added API ID to do call and relays on that weird legacy behaviour.

So nothing really changed except it works with hashing algo. There is one problem though - if passed API ID is not BA - it is not gonna work. This condition https://github.com/TykTechnologies/tyk/pull/2018/files#diff-651a84f8ad6a38c1f7ccdf63f2692410R328 makes sense only dashboard is sending right API ID, maybe we should remove spec.UseBasicAuth part from there.

The question is how would dashboard know the right API ID? Looks like asking for right API ID in this case is something extra as gateway's API already support key retrieval without API ID - in endpoint GET /tyk/keys/{keyName}?api_id={apiID} query string param api_id can be omitted and then our logic falls back to FallbackKeySesionManager https://github.com/TykTechnologies/tyk/pull/2018/files#diff-651a84f8ad6a38c1f7ccdf63f2692410R323

@buger
Copy link
Member

buger commented Dec 7, 2018

That's why I was thinking about extending Tyk Keys API, and for example, pass query attribute "?username=true" to the endpoint, and if it set, it will run logic similar to what you do in PR.

Dashboard can't know right APIID in advance.

@dencoded
Copy link
Contributor Author

dencoded commented Dec 7, 2018

makes sense to me. just to mention - we also need OrgID to generate right token and get key from storage. I guess the assumption with this approach would be that dashboard would be still passing key as OrgID + username and ?username=true, then we can extract OrgID from that value (it has fixed length) and generate right storage key

@buger
Copy link
Member

buger commented Dec 10, 2018

@dencoded makes sense to me

@dencoded
Copy link
Contributor Author

@buger I've added boolean parameter username to endpoints:

  • GET /tyk/keys/{keyName}
  • PUT /tyk/keys/{keyName}
  • DELETE /tyk/keys/{keyName}

username=true triggers to form correct key token with respect to current hash algo

POST /tyk/keys stays the same as we have everything to generate right key in request payload

api.go Show resolved Hide resolved
@buger
Copy link
Member

buger commented Dec 21, 2018

Exactly, instead orgid extraction logic you implemented. Also note that gateway api is an admin one, and we trust user input, so check if orgid valid also redundant.

@buger buger merged commit 7e70c43 into master Dec 24, 2018
@buger buger deleted the ba-getkey-fix branch December 24, 2018 13:39
buger pushed a commit that referenced this pull request Dec 24, 2018
Fix TykTechnologies/tyk-analytics#976 #2038

Added new attributes `org_id` and `username` (boolean) attributes, which should be passed together when you query for username.
buger pushed a commit that referenced this pull request Dec 24, 2018
Fix TykTechnologies/tyk-analytics#976 #2038

Added new attributes `org_id` and `username` (boolean) attributes, which should be passed together when you query for username.
buger pushed a commit that referenced this pull request Jan 13, 2019
Fix TykTechnologies/tyk-analytics#976 #2038

Added new attributes `org_id` and `username` (boolean) attributes, which should be passed together when you query for username.
buger pushed a commit that referenced this pull request Jan 13, 2019
Fix TykTechnologies/tyk-analytics#976 #2038

Added new attributes `org_id` and `username` (boolean) attributes, which should be passed together when you query for username.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants