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

add-auth-http-custom-support-in-rlc #1959

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Aug 8, 2023

fixes #1870
This PR is to support for http custom authentication like:

@useAuth({
  type: AuthType.http,
  scheme: "SharedAccessKey",
})

And use header like Authorization: SharedAccessKey <put my key here> to do the authentication.

As discussed in Azure/azure-sdk-for-js#25683, we will not add a commn policy in azure core. Instead, we will add a policy in the codegen side for http custom cases.

Right now, I am adding a policy in the client factory function createClient like

  client.pipeline.addPolicy({
    name: "customHttpAuthPolicy",
    async sendRequest(request, next) {
      request.headers.set(
        "Authorization",
        "SharedAccessKey " + credentials.key
      );
      return next(request);
    },
  });

And I am using KeyCredential as the credentials type in this case

export default function createClient(
  credentials: KeyCredential,
  options: ClientOptions = {}
): AuthHttpCustomClient;

Copy link
Member Author

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

do some self-reviews.

@qiaozha qiaozha marked this pull request as ready for review August 8, 2023 06:58
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I think the approach is solid.

Do we have any SDKs today that use this kind of auth that we can test with?

I was also curious if we wanted to support NamedKeyCredential as well though maybe that's too difficult to generically support at the moment.

@qiaozha
Copy link
Member Author

qiaozha commented Aug 8, 2023

Do we have any SDKs today that use this kind of auth that we can test with?

@xirzec I think event grid is using this way https://github.com/Azure/azure-rest-api-specs/blob/main/specification/eventgrid/Azure.Messaging.EventGrid/main.tsp#L6

@qiaozha
Copy link
Member Author

qiaozha commented Aug 9, 2023

Do we have any SDKs today that use this kind of auth that we can test with?

@xirzec I think event grid is using this way https://github.com/Azure/azure-rest-api-specs/blob/main/specification/eventgrid/Azure.Messaging.EventGrid/main.tsp#L6

Sorry, this is not a good example, eventgrid is using standard api key credential. I am not sure if it's their workaround or I am having some misunderstanding here. Confirming with @lmazuel offline.

Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @qiaozha for driving the conversations on this. I like the current approach

Copy link
Contributor

@MaryGao MaryGao left a comment

Choose a reason for hiding this comment

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

LGTM for the immediate problem and open with Option 3 once we notice this is a common requirement.

@qiaozha
Copy link
Member Author

qiaozha commented Aug 10, 2023

Confirmed with Laurent, that the eventgrid is not correctly written.

@qiaozha qiaozha merged commit 1e11a54 into Azure:main Aug 10, 2023
28 checks passed
@qiaozha qiaozha deleted the add-auth-http-custom-support-in-rlc branch August 10, 2023 01:28
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.

Support generating ApiKey with prefix
6 participants