search

OfficeDev / office-js-helpers

[ARCHIVED] A collection of helpers to simplify development of Office Add-ins & Microsoft Teams Tabs
MIT License
126 stars 56 forks source link

Fix `get` token recursion issue on TokenStorage for expired tokens #109

Closed meslater1030 closed 4 years ago

meslater1030 commented 5 years ago

This PR addresses issues #102 and #76 and it represents an alternative solution to PR #106.

When calling get on the TokenStorage class the method checks to see whether the token has expired. If the token has expired it calls super.delete(tokenKey) which lives on the parent Storage class. The Storage class delete method calls this.get(tokenKey). Since this in this case is the TokenStorage class we get an infinite recursion.

I don't believe it's good practice to delete the expired token within the get method of the TokenStorage class since it's a side effect. If the token is expired then subsequent actions should be left to the caller. The authenticator appears to be the only caller of this method within the library and so I've updated that code to delete expired tokens if necessary. I'm aware this is a public method on TokenStorage and I'm open to suggestions about other possible alterations if this represents a breaking change for end users. However - it appears that the check for an expired token is the only additional functionality TokenStorage provides and since it does that in a way that fundamentally does not work I suspect this is a net gain.

I've added tests for the change.

I've also updated the typings for tapable as per the recommendations here: https://github.com/webpack/tapable/issues/53. A new install of this project otherwise throws errors.

Finally, my tests revealed that the reviver deserializer in the Storage class relies on an outdated REGEX matcher for ISO strings. I've updated that as well.

Thanks for your help and let me know if you have any questions.

msftclas commented 5 years ago

CLA assistant check
All CLA requirements met.

meslater1030 commented 5 years ago

@sumurthy or @Zlatkovsky can I get a review on this?

Andruha4u commented 5 years ago

Hello, currently using this one and will appreciate merge it to master, thank you ahead. @meslater1030 @sumurthy @Zlatkovsky

super2ni commented 5 years ago

Hey there @meslater1030 ,

Sorry for the noobish question but I am not very confortable with Git. Seems like your PR corrects a bug I have on IE and I would like to install your PR has a NPM dependency.

What is the correct npm command to do that?

Thank you, Denis

techieanalyst commented 5 years ago

I'm also having issue which has an error saying:

unable to delete 'Microsoft' from storage

When will this fix be merged into master?

Zlatkovsky commented 5 years ago

Adding @LouMM , @sumurthy as the maintainers of this repo.

tomaszzmuda commented 5 years ago

It would be nice if this will work soon :)

Mathyaku commented 4 years ago

I'm still having this problem. has Anyone a solution for that ?

meslater1030 commented 4 years ago

Closing due to lack of response from maintainers.