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

feat(auth): introduce WithinBody option for AuthMode #1297

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Aug 6, 2021

fixes #1035

Implementation is done by:

  • adding a new auth mode
  • adding data as the return type for createAuth
  • expose data from transporter
  • serialize transporter data
  • map api key back to query parameter if GET

This doesn't automatically switch to body if the key is too long, the option authMode needs to be set

algoliasearch('', '', { authMode: AuthMode.WithinBody })

TODO

  • tests
  • validate this works

fixes #1035

Implementation is done by:
- adding a new auth mode
- adding data as the return type for createAuth
- expose data from transporter
- serialize transporter data
- map api key back to query parameter if GET
@@ -40,6 +40,8 @@ export function retryableRequest<TResponse>(
request.method !== MethodEnum.Get
? {}
: {
// if AuthMode.WithinData, we forcibly map apiKey back to a query param
'x-algolia-api-key': transporter.data?.apiKey,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might be implemented wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean by "wrong", but I have a extra question: is the idea here to move auth of all non GET requests to the body?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 6, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9281ee9:

Sandbox Source
javascript-client-app Configuration

Copy link
Contributor

@tkrugg tkrugg left a comment

Choose a reason for hiding this comment

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

I agree with the change, but can we have a test securing this behaviour?

@Haroenv
Copy link
Contributor Author

Haroenv commented Aug 6, 2021

it's in the todo @tkrugg

@Haroenv Haroenv marked this pull request as draft August 6, 2021 11:36
@jpreynat
Copy link

@Haroenv Any news on this PR?
It'd be great to have this released to fix #1035

@Haroenv
Copy link
Contributor Author

Haroenv commented Jan 3, 2022

Sorry, there was too many places to change in this unplanned PR, but withinHeaders should be an in-between solution, as it has a higher limit (but doesn't avoid a cors preflight request) @jpreynat

@jpreynat
Copy link

jpreynat commented Jan 3, 2022

Thanks for the suggestion @Haroenv. But sadly we already tried using withinHeaders and we have some cases where the limit is too small for us, preventing us from switching from version 3 to version 4.
Are there any plans on your side to have the withinBody option released at some point?

@Haroenv
Copy link
Contributor Author

Haroenv commented Jan 3, 2022

At the moment it's not yet on the roadmap, as I made this pull request a while ago in an experiment. I will however ask the current owners of this repo whether they can take a look (@shortcuts, @millotp)

@humanbagel
Copy link

@Haroenv @shortcuts @millotp @tkrugg can this prioritized now? it's really preventing enterprise users from the full performance benefits of algolia if we can't use long secured api keys from the browser

@Haroenv
Copy link
Contributor Author

Haroenv commented May 20, 2022

Sorry, I'm not on this team anymore @humanbagel, but I do know that algoliasearch v3 had this feature, so you can use that before it's prioritised to be fixed. Someone who's actually on the api clients team can give more information on prioritisation

@BohdanYavorskyi
Copy link

@nunomaduro what's the issue that this PR not merged? Enterprise-level apps need that a lot :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discussion: very long secured api-keys cause problems while searching
5 participants