Skip to content
This repository was archived by the owner on Apr 20, 2019. It is now read-only.

Inject credentials from batch endpoint if applicable#87

Open
alphapseudo wants to merge 2 commits intooutmoded:masterfrom
alphapseudo:allow-credential-reuse
Open

Inject credentials from batch endpoint if applicable#87
alphapseudo wants to merge 2 commits intooutmoded:masterfrom
alphapseudo:allow-credential-reuse

Conversation

@alphapseudo
Copy link
Copy Markdown

This pull request serves as an optimization for batch requests that require authentication.

Expected Result:
If the batch endpoint requires authentication, it should re-use those credentials for any of the subsequent injected requests. In other words, it should only check the session storage once for all the requests in a batch.

Actual Result:
The subsequent requests ignore the initial credential validation and performs a session lookup for the n requests.

Reproduction Steps:

  1. Apply an authentication strategy to your batch endpoint (e.g.)
    options: {
      batchEndpoint: '/api/batch',
      tags: ['batch'],
      auth: 'session'
    }
  1. Define an endpoint that requires the same authentication strategy (e.g.)
  {
    method: 'POST',
    path: '/api/users',
    config: {
      auth: {
        strategy: 'session',
        scope: ['users:read']
      }
    },
    handler: UserController.createUser
  }
  1. Make a POST request with a payload (e.g.)
[ { method: 'POST',
    path: '/api/users',
    payload: { first_name: 'Andrew', last_name: 'Fake' } },
  { method: 'POST',
    path: '/api/users',
    payload: { first_name: 'John', last_name: 'Doe' } } ]
  1. Observe session storage. The session lookup is linear to the number requests instead of constant.

Copy link
Copy Markdown
Contributor

@cadecairos cadecairos left a comment

Choose a reason for hiding this comment

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

Hi @alphapseudo, Thanks for submitting this pull request!

Before this can get merged, can you please write a unit test for this change?

@hueniverse
Copy link
Copy Markdown
Contributor

@cadecairos looks like @alphapseudo moved on. Either finish the work yourself or close the issue.

@alphapseudo alphapseudo force-pushed the allow-credential-reuse branch from b08cb1c to fb9da91 Compare December 5, 2018 05:34
@alphapseudo
Copy link
Copy Markdown
Author

@cadecairos sorry about the delay, I added a test in fb9da91

@hueniverse
Copy link
Copy Markdown
Contributor

@cadecairos Can we bring this to a conclusion?

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