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

Using https:// as part of the allowedHosts shouldn't be an issue #939

Closed
maisarissi opened this issue Dec 14, 2023 · 11 comments · Fixed by #1051
Closed

Using https:// as part of the allowedHosts shouldn't be an issue #939

maisarissi opened this issue Dec 14, 2023 · 11 comments · Fixed by #1051
Assignees
Labels
enhancement New feature or request WIP
Milestone

Comments

@maisarissi
Copy link

When using Azure Identity to create an authProvider and adding a allowedHost as the following:

final String[] allowedHosts = new String[] {"https://graph.microsoft.com"};

makes the client creation fail with "empty token" error.

I should be able to add allowedHosts with and without https://

@baywet baywet added this to Kiota Dec 14, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Dec 14, 2023
@baywet baywet added the enhancement New feature or request label Dec 14, 2023
@andrueastman
Copy link
Member

@baywet Should we also move this one up to include all the other languages' AllowedHostValidators to try to strip out URI semantics before comparing the hosts?

@baywet
Copy link
Member

baywet commented Dec 15, 2023

@andrueastman since this is not impacting generation I meant to create issues in the other abstractions repos. But I had forgotten. This is now done, thanks for the nudge. This should be considered a low priority item for now, and http (without the S) should also be considered, as well as different casings.

@andrueastman
Copy link
Member

@andrueastman since this is not impacting generation I meant to create issues in the other abstractions repos. But I had forgotten. This is now done, thanks for the nudge. This should be considered a low priority item for now, and http (without the S) should also be considered, as well as different casings.

Thanks for confirming here

@XdpCs
Copy link

XdpCs commented Jan 16, 2024

I believe there may not be a need to make changes to this issue, as it introduces potential confusion regarding the concept of hosts.

@baywet
Copy link
Member

baywet commented Jan 16, 2024

@XdpCs can you expand on the scenario you are thinking about please?

@XdpCs
Copy link

XdpCs commented Jan 17, 2024

@baywet In my opinion, allowedHosts should only include the hosts, not the entire URLs. Including ‘http’ or ‘https’ would involve the entire URL, while hosts are just a part of the URL.

@baywet
Copy link
Member

baywet commented Jan 17, 2024

I understand the concern about the naming. The reason why Maisa created this issue is because we've observed it's a mistake (leave the scheme) that's commonly made and often trips people when they first try kiota.
Instead of stripping it automatically, we could throw an exception with an explicit message. What do you think about this alternative?

@XdpCs
Copy link

XdpCs commented Jan 17, 2024

I agreed on this alternative. However, if someone directly utilizes the specified link in the kiota-abstractions-go repository, there might be compatibility issues. This function will add an error param for return.

@XdpCs
Copy link

XdpCs commented Jan 17, 2024

If you don't think this is a problem, I will provide the Go version.

@baywet
Copy link
Member

baywet commented Jan 17, 2024

We could always provide an "overload" with slightly different name that also returns an error.

@andrueastman
Copy link
Member

@baywet @sebastienlevert Created microsoft/kiota-typescript#1039 for TS as this looks like an oversight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants