Replace ngCookies with sessionStorage #95
Conversation
Remove ngCookies dependency and replace it with sessionStorage. Cookies are problematic when dealing with platforms such as Cordova, Ionic or PhoneGap, since cookies aren't available on these platforms. The Web Storage API provides a robust implementation available on all platforms (http://caniuse.com/#feat=namevalue-storage).
|
@jonkoops thanks for your contribution. There are some issues with your implementation mainly because you're removing the option for using cookies for storing data on web applications. We choose to implement this library with cookies, mainly because it is the standard for storing sensible data such as tokens. Web storage provides no additional protection against harmful attacks. For instance, by using cookies with |
|
I don't think the removal of cookies has that severe of an impact impact. To address some of the points you've mentioned:
When using session storage the content of the token is never send in any request. The best security you can get here is never sending your token in the first place, and session storage does that. This is especially true for users of this library that have not implemented HTTPS on their site (they still exist unfortunately).
This is true, however I doubt a lot of users actually make active use of this feature. This is one of the reasons I considers this PR to be breaking. However I believe the benefits of removing the dependency on cookies outweigh the cons.
Web Storage isn't vulnerable to CSRF attacks because it never sends anything to the server. It is however vulnerable to cross-site scripting (XSS) attacks like any other JavaScript code. This can be mitigated by implementing a Content Security Policy (CSP). I've taken a look at the proposed solution in #97 and I think it is a good solution. However I think the end game is that users of this library want things to 'just work' and this solution makes it more complex for them to use this library. Obviously I have taken the decision to remove cookie based storage a bit too hastily seeing there are solid arguments for keeping it around. So I would like to propose a middle ground. I could update this pull request to work both with cookies, session and local storage (maybe even a memory fallback) by adding an The default configuration would look something like this: myModule.config(OAuthTokenProvider => {
OAuthTokenProvider.configure({
name: 'token',
storage: ['cookie', 'session', 'memory'],
options: { secure: true }
});
});The behaviour here is that each storage method that is passed is looked at until one that is supported is found, if no suitable storage method is found we could throw an error to inform the implementor. This is inline with the 'just works' mentality, possibly removes external dependencies and makes it easy for users of this library. |
|
I kind of agree with the "just works" mentality but I would argue that it would be out of the scope of our effort to simplify oauth2 implementation. It would be required to add support to several issues related with storage services, such as safely stringify malformed json objects and other security. This would increase the library size and complexity, by adding to much abstractions. I think we should reach a more modular approach instead of adding new over complex and unnecessary features, such this one, in order to improve the project's maintainability. |
|
Additionally adding support to cookies the way you suggest is not a straight forward implementation. Since cookies only accept strings, you would need to create a cookie reader and cookie writer, to manipulate the passed data structure, encoding and decoding it to an URI component and additionally add support to cookie specific options. Basically you would be implementing |
|
I don't believe supporting these options out off the box coincides with 'over complex and unnecessary features'. These are real issues people are encountering, as shown by various bug reports, pull requests and forks of the project. The solution in #97 doesn't provide a solution that works out of the box like a lot of users need. Rather it exposes an interface for users to implement a storage method without the ability to easily fall back to other storage methods if necessary (think falling back to a cookie when web storage is unavailable). I would like to implement the pull request as discussed in my previous comment and we can deal with any problems along the way. As for the issues regarding the complexity of cookies, I am aware off them but there are plenty of libraries we could use to make this easier to implement. And since these are probably really small we could include them in the minified js to make it easier for users. |
|
@ruipenso I would love to have your opinion on this as well. |
This removes the ngCookies dependency and replaces it with sessionStorage. Cookies are problematic when dealing with platforms such as Cordova, Ionic or PhoneGap, since cookies aren't available on these platforms. The Web Storage API provides a robust implementation available on all platforms (http://caniuse.com/#feat=namevalue-storage).
I don't expect this to break any backwards compatibility, but I think this pull request should still be considered a breaking change. So that would justify a major version bump.
As a sidenote, I believe a lot of users of this library would want this implemented as can be told by the various pull requests, issues and forks: #89, #35, #19. It was mentioned in one of the threads that an agnostic storage solution (being able to store in session or persist locally) would be preferred but I believe that could be added at a later point in time without a lot of effort.