Turn library state into encoded string that contains guid and timestamp

[jasonnutter]

Proof-of-concept for fixing #1259. Per our design discussion, this would turn the value we pass as state into a base64 encoded object containing a guid and original timestamp of the request (and eventually it will contain the request method to address loginRedirect bugs).

Based on this PR to ADAL: https://github.com/AzureAD/azure-activedirectory-library-for-js/pull/898/files

TODO:

• Confirm backwards compat is needed.
• Finalize keys
• Tests
• Error handling
• Test with tab suspending

[DarylThayil]

what can be tested?

[jasonnutter]

what can be tested?

Unit tests for RequestUtils (and other tests that rely on state), and manual testing of the tab suspension scenario.

[DarylThayil]

Any concerns on char length with state?

[jasonnutter]

Any concerns on char length with state?

Until we add more values to the object, I think it should be a consistent 88 characters. That is definitely a bit longer than before, but I don't think its too long. We'll want to double check that as well.

Example:

eyJzdGF0ZSI6IjM0ZDBkNTZjLTEyMWUtNDkxZS1iNTg3LWVjODBiNzRiZjk3MiIsImlhdCI6MTU4NDU3NjQxMH0=

[DarylThayil]

Question, the part we are validating is still the full string we are sending right? we are not cracking it open when it comes back and saying does state match stateobj.state?

[jasonnutter]

Question, the part we are validating is still the full string we are sending right? we are not cracking it open when it comes back and saying does state match stateobj.state?

No, this would still decode the state object (which we need to do anyway) and then does checks with the guid.

[sameerag]

Can we add the 'requesttype to thestate` and calculate the encode decode length? we need that to fix popup scenarios in teams anyway.

[DarylThayil]

Question, the part we are validating is still the full string we are sending right? we are not cracking it open when it comes back and saying does state match stateobj.state?

No, this would still decode the state object (which we need to do anyway) and then does checks with the guid.

Seems like whatever we send to state (an encoded string) is what we should validate comes back. For instance if I create an object

const stateToSend  = {
   state: "a",
  otherItem: "b"
}

encode(stateToSend) = abcdefg

and then another state comes back with the same state parameter but maybe different other parameters

stateThatComesBack = hijklmnop

decode(statethatComesBack) = {
     state: "a",
     otherItem: "c"  
}

This decoded State would evaluate to true, but the Other Item would not which could be a potential surface area for attack. I would imagine the check should be abcdefg === abcdefg and that anything we do when we decode it from there on can be trusted

[jasonnutter]

@DarylThayil Yeah, that's fair.

[sangonzal]

Looks good. It might be useful to have some logging around what ts is added and returned in state object for debugging.

[coveralls]

Coverage increased (+0.01%) to 76.669% when pulling 9b568bf on encoded-state-iat into d558bdb on dev.

[pkanher617]

+1 for naming changes. Looks good now. I'm assuming you tested this with the react-sample-app but it may be good to add an e2e test or two for this.

[jasonnutter]

@pkanher617 I'm not sure how to test something like tab suspending in an automated environment, unfortunately. I think we'll have to rely on unit tests and manual tests for that specific scenario.

[pkanher617]

@pkanher617 I'm not sure how to test something like tab suspending in an automated environment, unfortunately. I think we'll have to rely on unit tests and manual tests for that specific scenario.

Can't we just add a test in the basic use case to test that the state changes are working as expected? Or do you think unit tests are enough to test this? We don't necessarily need the specific case for #1259 in end-to-end, but something to test with the service would be good.

[jasonnutter]

Can't we just add a test in the basic use case to test that the state changes are working as expected? Or do you think unit tests are enough to test this? We don't necessarily need the specific case for #1259 in end-to-end, but something to test with the service would be good.

I would expect the existing e2e tests to fail if the state changes weren't correct, yeah? As the library should throw if state is invalid.

[jasonnutter]

Add a test to mimic tab suspending, which I verified fails if the old logic is used.