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

Support for setting headers dynamically #151

Closed
SecretBase opened this issue Oct 30, 2019 · 4 comments · Fixed by #152
Closed

Support for setting headers dynamically #151

SecretBase opened this issue Oct 30, 2019 · 4 comments · Fixed by #152

Comments

@SecretBase
Copy link
Contributor

Background

I have a oauth2 authorization server to let registered oauth2 clients performing client credential grant.
https://tools.ietf.org/html/rfc6749#section-4.4

I want to securing unleash client endpoint by verifying a jwt from the Authorization header.

Problems

Since the unleash client only initialize once. The customHeaders still remain static the rest of the up coming request.

Proposed Solution

I was wondering adding a feature to setting headers dynamically upon request. Like accepting a callback that return a Promise resolve as a header object. By doing that I can get a new access token when there are requests to the unleash server's client endpoint.

example

const { Unleash } = require('unleash-client');

const instance = new Unleash({
    appName: 'my-app-name',
    url: 'http://unleash.herokuapp.com',
    headers: async () => {
        const { access_token, token_type } = await oauthToken(); // Request to an oauth2 authorization server
        return {
            Authorization: `${token_type} ${access_token}`,
        };
    },
});

I will be so grateful anyone can give me some guidance/advice ? So I can make a PR for that.

@ivarconr
Copy link
Member

Hi, thanks this is aligned with a similar problem I want to solve soon.

I would hovever like the headers to be a function (or a static object as today), and not require it to be a promise. You are free to use async functions inside that function of course.

@SecretBase
Copy link
Contributor Author

SecretBase commented Oct 31, 2019

@ivarconr thanks for the reply

I would hovever like the headers to be a function (or a static object as today)

I am not sure I get that or not. I guess you mean better to not break the current interface. If the case, we can make the customerHeaders accept a object or function. So it won't be a breaking change for the users. So the interface will look like this

export interface CustomHeaders {
    [key: string]: string;
}

export type CustomHeadersCallback = () => Promise<CustomHeaders>

export interface UnleashConfig {
    // ..others config
    customHeaders?: CustomHeaders | CustomHeadersCallback;
}

and not require it to be a promise.

May I ask for more context/concern on this one ? Since Promise is quite common pattern for async operations like network request. Although we can made that a callback like this

const instance = new Unleash({
    appName: 'my-app-name',
    url: 'http://unleash.herokuapp.com',
    headers: done => {
        oauthToken()
            .then(({ access_token, token_type }) => {
                done(null, {
                    Authorization: `${token_type} ${access_token}`,
                });
            })
            .catch(done);
    },
});

Hi, thanks this is aligned with a similar problem I want to solve soon.

I would love to listen to the problems and see what we can do for it

@ivarconr
Copy link
Member

ivarconr commented Oct 31, 2019

I thought a bit more on this, and you are right. It should be a promise and I think it should be a separate configuration option (and not try to retro-fit it in to existing customHeaders option).

Would a good name for this option be customHeadersFunction?

If this option is defined it should take precedence over the customHeaders option.

@SecretBase
Copy link
Contributor Author

sound good to me.

Will create a Pull Request within couple days.

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 a pull request may close this issue.

2 participants