Fix/id token issuer #2
Conversation
SayazhanBos
force-pushed
the
fix/id_token_issuer
branch
from
074a4f3 to
b3036eb
Compare
SayazhanBos
force-pushed
the
fix/id_token_issuer
branch
from
b3036eb to
4674134
Compare
There was a problem hiding this comment.
Please update the comments as described. We're making a pretty significant change from the source project and we need to describe why those changes are being made. Especially since we've had to make numerous changes to this code already, so it seems likely that we may need to return to again some time.
| @@ -160,15 +160,15 @@ private static JSONObject parseJwtSection(String section) throws JSONException { | |||
| return new JSONObject(jsonString); | |||
| } | |||
|
|
|||
| static IdToken from(String token) throws JSONException, IdTokenException { | |||
| public static IdToken from(String token) throws JSONException, IdTokenException { | |||
| String[] sections = token.split("\\."); | |||
|
|
|||
| if (sections.length <= 1) { | |||
| throw new IdTokenException("ID token must have both header and claims section"); | |||
| } | |||
|
|
|||
| // We ignore header contents, but parse it to check that it is structurally valid JSON | |||
There was a problem hiding this comment.
Please add an annotation to this comment that notes it is the original project comment, but that you've changed the code so that it is no longer valid. Then add a comment explaining that we are now getting the header contents and why (Azure sends the nonce through the header).
There was a problem hiding this comment.
changed the comment
| final String nonce = JsonUtil.getStringIfDefined(claims, KEY_NONCE); | ||
| String tempNonce = JsonUtil.getStringIfDefined(claims, KEY_NONCE); | ||
| if (tempNonce == null) { | ||
| tempNonce = JsonUtil.getStringIfDefined(headers, KEY_NONCE); |
There was a problem hiding this comment.
Please add a comment that we observe the nonce coming from Azure in the header and not the claim.
There was a problem hiding this comment.
added the comment
| String[] sections = token.split("\\."); | ||
|
|
||
| if (sections.length <= 1) { | ||
| throw new IdTokenException("ID token must have both header and claims section"); | ||
| } | ||
|
|
||
| // We ignore header contents, but parse it to check that it is structurally valid JSON | ||
| parseJwtSection(sections[0]); | ||
| // We are using headers to get nonce field from it and also parse it to check that it is |
There was a problem hiding this comment.
I'm sorry, I really think it's important to preserve the original comment (because it's coming from the source project) so that it's easier to "work backwards" if we need to in the future.
Something like:
// (AMN code changes invalidated this comment, 04-2024) <original comment>
Then:
// Get header contents because we have observed Azure sending the nonce in the header
Checklist
Motivation and Context
Description