Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
adds host and scheme validation for access token providers #1051
adds host and scheme validation for access token providers #1051
Changes from 9 commits
8eb60e7
c2810cc
9e75ba0
df9f112
c1e9ce1
0b2679a
e8cf09b
062fda0
7e5ff35
9a3cedc
89ee6b4
3848118
1aad180
757dcaf
14314aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a class instead of simply exporting a util function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we need something to hold the list of valid hostnames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I think that would be necessary for Java or dot net.
Currently we maintain this as util functions:
https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/3eab6727853b1977bd8046fa3bab8e95dafe6f40/src/GraphRequestUtil.ts#L68
https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/9441382f7963fa782cd30a103af2632d7585e0a0/src/Constants.ts#L28
Related to the comment below we could validate the url using a single util function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main difference being the list of hosts is maintained on a global context object, which is a mix of everything (request options, hosts, etc...) vs a self contained (encapsulated) object.
https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/3eab6727853b1977bd8046fa3bab8e95dafe6f40/src/IContext.ts#L29
I do have a preference for encapsulation over a mix "context" object. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am with you about not storing hosts in context object.
When I thinking of the flow for Graph JS SDK we would go
The user_custom_hosts would not contain the Graph hosts.
At the time of initialization I would append the Graph hosts to this user_custom_hosts.
Is the use case valid?
If that is the case, the allowed hosts should be public property so that it can be modified.
In that case, the allowed hosts validation can be a util function which can be passed the allowed hosts. We won't be storing it in the Context.
What I missing how is the
AllowHostsValidator
class going to be accessible to Graph JS SDK to store its hosts and then passed to theAuthenticationProvider
?We can continue this discussion in #1063 and make more updates accordingly. This conversation need not block this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @Ndiritu for PHP, sorry I already had merged your PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also CC @samwelkanda for the ongoing work in #925
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about have a public function
AppendHosts
in the hosts validator class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we add an append host (which could be done by Get, append to the result, set) we'd also need to add a remove host method for consistency. All of that can already be done with the current API surface, I don't think it'll add much value to the current offering.