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
PKCE implementation #205
PKCE implementation #205
Changes from 36 commits
b0624fa
8a4e4c8
c140fd0
839139c
23857b6
3901d65
2e89506
1f26bfa
8421b1c
6ee3148
0f3cc7b
aa9ccc7
06b8dfa
7650c24
d53cc8c
71dc682
5d3b428
caf4e4e
6958175
3739e47
173e9af
5d68ca5
3e41320
ec2b368
a6ebafa
bbcf753
5cc5c98
4f46c08
a9931f2
6beae1d
cd5e5da
0746ffc
8a92988
1cbfa75
b078905
9c308b1
ce798ce
1c99d55
6e3c197
9ddc284
82e1ca9
5589c7b
85e95eb
365d129
0322b0c
e386714
dc1adca
4f11f53
ce15188
d384a71
d145903
0ecc4fc
e13491e
1cd3306
d8cfe0a
d702a20
29c1974
b438da1
24deee5
c2f1eb1
be807b7
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.
Just to clarify - the resolved return value from
token.parseFromUrl
will be anaccessToken
and/oridToken
. Theauthorization_code
will never be available to the developer, correct?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.
correct
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.
Are we sure it will be 'Content-Type' and never 'content-type'? IIRC the spec is not case sensitive.
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 header can be either, but looking up the key
'Content-Type'
in this object is case sensitive. We should probably check'Content-Type'.toLower()
too.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.
Bump ^
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 can definitely make this change. But in reality we are the only users of this code. We use it as a pluggable utility wrapper around another xhr lib. Anything we do here, we must duplicate that logic in our jqueryRequest and reqwestRequest objects as well. Previously EVERYTHING was JSON encoded. I am creating a small pinhole for the token POST to succeed
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.
Shouldn't we check these values to be safe? If our dependency on cross-fetch, an application and/or plugin modifies the headers to be lowercase, this check will fail.
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.
style-nit: Space after
:
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.
FYI: According to the spec this value should be checked against the original redirect URI:
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 believe the server is doing this check. This validation here is around the API call - it is saying
redirectUri
is a required parameter to the api (the server will fail the call if it is not present).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.
+
and/or id token