Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Draft
Haroenv wants to merge 1 commit into v4
base: v4
Choose a base branch
Loading
from feat/auth-within-body

Conversation

Copy link
Contributor

@Haroenv Haroenv commented Aug 6, 2021
edited
Loading

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

PuKoren, sjerdo, and ffffranklin reacted with thumbs up emoji
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

@Haroenv Haroenv Aug 6, 2021

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

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?

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

@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?

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
Copy link

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

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

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?

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)

jpreynat, PuKoren, and humanbagel reacted with thumbs up emoji

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

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

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
Reviewers

@tkrugg tkrugg tkrugg requested changes

+1 more reviewer

@Vishalbhilavala Vishalbhilavala Vishalbhilavala approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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

AltStyle によって変換されたページ (->オリジナル) /