Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Setting cookie attributes in $cookieStore #2459

Closed
wants to merge 6 commits into from
@Illniyar

As a response to issues: #1786,#950,#1918,#1320
This PR enables setting a cookie's path and expires attributes using the $cookieStore service.
$cookies was not changed, since it's API is too strict, I could find no way to implement it without causing breaking changes, or using some terrible hacks.
I would suggest adding a new service to the module to support more advanced features (such as Json $watch expression, which should be more useful then binding the $cookies object directly), but this would probably require a bit of research regarding use cases.

Some things to note regarding the implementation:

  • urlroot (karma property) was added to all unit tests - to facilitate testing of different cookie path
  • Backward incompatible change (bad fixing a bad behaviour): delete cookie now deletes cookies set with a path other then / (or baseHref) that is before the fix, if you had a cookie set on path /karma (if it was added by the server for instance), you would be able to see it, but deleting it will fail silently (and will not delete) delete cookie now deletes duplicate cookies with the same name (if they have different paths)
  • Due to the fact that no browser allows inspecting the Path and Expiration date of a cookie, I chose to add a private _setCookie function inside the $browser module which I override in unit tests. Not the most elegent solution, but the only other way to check Expiration is to mock the document object and change how document.cookie works, and that would have taken me too long for very little benefit.

Design decisions:

  • to change the path and/or expiration date of a cookie, you can pass an options object as the third argument to cookies(name,value) the options object can override Path and Expires when adding a value, all other options variables are ignored (but are allowed, I.E. no validation is made). You can pass either path, expires,none or both.
  • the path option is checked to be a string and be partial to window.location, otherwise, the default path is used. (warning is logged)
  • the expires option is checked to be a Date and in the future, otherwise no expiration is set. (warning is logged)
  • at the moment no support for non-Date expires is supported (for instance such as maxAge, or a number to indicate number of millesconds for cookie to last)
  • you can now delete a cookie on a specific path by calling cookie(name,undefined,options), with options containing path
  • only the $cookieStore service will support putting cookies with different path/expiration (as well as deleting certain paths. this is to support backward compatability with $cookies.

Notes:
if the value passed to browser.cookies is not a String, it fails silently.

This is my first time contributing here (and on a Major open source project), I hope this can be incorporated.
I'm more than happy to make changes if something is wrong, so please let me know.

Illniyar added some commits
@Illniyar Illniyar feat(ngCookies): support passing path/expires attributes to cookies
$cookieStore service had no way to set cookie attributes (such as expires).
The api for $cookieStore.put and $cookieStore.remove now has a third argument-
options,which allows setting cookie attributes, or deleting cookies under a
specific path.
- to change the path and/or expiration date of a cookie, you can pass an
  options object as the third argument to cookies(name,value).
  the options object can override Path and Expires when adding a value.
  You can pass either path, expires,none or both.
- the path option is checked to be a string and be partial to window.location
  otherwise, the default path is used. (warning is logged)
- the expires option is checked to be a Date and in the future,
  otherwise no expiration is set. (warning is logged)
- you can now delete a cookie on a specific path by calling
  cookie(name,undefined,{path:x})

The $cookie service remains unchanged since supporting cookie attributes
would mean serious backward comptability issues.
Foundations were put in place to make supporting $cookie easier.
GENERIC TEST CHANGE:
To support checking cookie path, all unit-tests now run on path "/karma/tests"
Documentation was updated for both $cookie and $cookieStore with examples.

closes	#1786, #1320
BREAKING CHANGE: As part of the change, deleting a cookie  now deletes
   cookies set in multiple paths (and duplicate cookies if exists).
   Previously only cookies set in the / path were deleted.
   Since it is not intutive, if this change breaks someones code, it is
   probably as an accidental side-effect.
   It is reasonable to assume that most people actually wanted to delete the
   cookie even if it wasn't set in the same path (since they can see it).
   So while this is a breaking change, it fixes bad behaviour.
   If needed, you can delete the cookie in a specific path using $cookieStore.
a4ed23c
@Illniyar Illniyar feat($cookieStore): $cookieStore.get now parses blank string as blank…
… string

closes #1918
8efadf8
@Illniyar

I also signed the "Contributor License Agreement" electronically, I think (it redirect to nothing :/) .
Using the email used here.

@petebacondarwin

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented
@petebacondarwin

Internet Explorer 8 & 9 don't like this: http://ci.angularjs.org/job/angular.js-pete/90/

@petebacondarwin

By the way, you appear to have modified all functions to have a space between function and (), which makes it look like you have far more changes than there really are. I suspect that you didn't purposefully do this for some idealistic aim, but that perhaps your editor did it for you?
For consistency with the rest of the code base I would suggest you revert that formatting in the PR.

@petebacondarwin

Can you explain why you have to expose and mock _setCookie in the service rather than simply injecting document and accessing document[0].cookie in the specs?

@Illniyar

Function spaces- I suspect the editor did that, I've hopefully reverted that in the following commit.

the problem with accessing document.cookie is that you can't read the Path or Expiration date of a cookie using javascript. So I couldn't have verified that path and expiration was done properly. That's why I need to capture the calls before they reach document.cookie (or mock document.cookie itself, which is a lot of work for little gain).

For example, if I set document.cookie = "cookie=numnum;path=/;expiration=111" when I read document.cookie, I'll get "cookie=numnum".

In regards to why injecting a mock document isn't easy, it's mostly because the tests are already using document, and I would need to mock all the APIs that are used (or alternatively use something like harmony-proxies, which don't work on all browsers).

I've fixed the IE issues that appeared on the tests, however for some bizzare reason, my IE suite get stuck at test 749, and I'm unable at the moment to complete a full suite on IE. since I haven't touched those tests, I'm mostly sure it works (and the fact they didn't appear on the cross-browser hudson also raises my hopes here).

@Illniyar

Can I ask why "PR is squashed into one commit per logical change" and "PR's commit messages are descriptive" are not checked, did I miss something?

@petebacondarwin

@Illniyar -
Thanks for working on the PR.

To answer your question about the tasks:
Two of your four commits are not in the correct format and in any case they should probably be squashed into the other two commits.

I would like to look at the mocking issue a bit further because I don't like the idea of creating backdoors just for testing here. I'll give it a go on with IE on my machine.

@Illniyar

What messages do you put for commits that fix regressions in previous commits? how do you "squash" two commits?

In regards to mocking cookies, what browser support is needed? I can think of a way of doing it, but I need getter/setter support, and I'm not sure what browsers does angular.js need to support (including version numbers for firefox/chrome etc...).
The main problem now is that cookie doesn't work like a normal field, there's no way to mock it without getter/setter support, and if there's one browser that it is tested on and doesn't have such support the tests will fail.

@Illniyar Illniyar closed this
@Illniyar Illniyar reopened this
@Illniyar

Thank you pkozlowski-opensource. I'm not really sure how to squash things now though, should a new branch be created with a new PR? I can't see a way to squash commits on the same branch/repository you are currently working on.

@Illniyar

I've commited a fix that will remove the _setCookie backdoor. Hopefully this should work in: IE8+,FF4+,Chrome and Opera12+.

@petebacondarwin

@Illniyar - don't worry about squashing. I can rebase if and when we merge.

@petebacondarwin

@Illniyar - I am having some issues with this:

http://plnkr.co/edit/6RuykjI6U1AJec0GMwPE?p=preview

It doesn't seem to actually store the cookie in the browser.

Moreover, my understanding is that you remove a cookie from the browser by setting its expiration value to now. You are not doing this.

Any thoughts?

@Illniyar

I'm not sure what is going on there. If I take the exact same code and put it on directly on a server, it works. Something in Plunker is not working properly, probably there is some kind of permission issue that block the cookie from being set,maybe something to do with the iFrame?

In regards to removing cookie, we set the date to "expires=Thu, 01 Jan 1970 00:00:00 GMT" in all places where we try to delete the cookie, I'm not sure what your seeing that is different.

@Illniyar

any news here?

@karlfreeman

Would love to see this be pulled in! Thanks for the request @Illniyar :+1:

@karlfreeman

@Illniyar Have you considered also adding secure to the cookie store?

@Illniyar

It should be relatively easy to add (along with domain specific cookies ) as the additional options argument provides a lot of lieniency. However there were several reasons why I didn't:
1) There were no issues open for this (or for adding domain).
2) I did not want to overload a single commit with too much functionality.
3) The testing for this might require quite a bit of effort.

@karlfreeman

I guess we need to hear from @petebacondarwin about how much $cookieStore should do. Would be great to have feature parity with other js cookie libs.

@karlfreeman

#950 does mentions a couple things like secure

@IgorMinar
Owner

LGTM for this commit. @petebacondarwin can you merge it in please?

Sorry about that, in retrospect, I should probably have made two different pull-requests/branches. this commit was commited on top of the other cookie changes (and not the other way around).

@IgorMinar
Owner

I looked over the commit a4ed23c and I like the direction, but it still needs some major refactoring:

  • the use of $cookieOptionHash is really hackish, I think that we should make it possible to set the option via $cookies service. How about $cookies.$set(name, value, options) or $cookies(name, value, options) both are backwards compatible.
  • I'd like to move all of the cookie related code out of $browser into the angular-cookies module. long time go other stuff depended on $browser, but currently cookies is the last major piece that holds us from killing $browser. This change could happen as part of this refactoring (maybe as a separate commit though).

What do you think about these two big changes? Would you be up for addressing them?

If you sign up for the $browser refactoring and are capable of making the changes in a timely manner, I'll make sure that this change lands in 1.2 (we'd like to feature freeze 1.1.x within a week).

@Illniyar

The $cookies service (as opposed to the $cookieStore service) was appearently desgined to work as a hashmap, I.E. people might use the cookies hashmap directly (see the example I added to the docs to see how it might be used).
Which means that adding any variable to it will cause backward incompatability with anyone who iterates through the entire hashmap.
Since it is returned directly by the factory, we also can't add multiple functions to the $cookie service (as opposed to the $cookieStore service).

I didn't consider returning $cookies as a function, I can't think of a reason why it'll break backward compatability, can you? if not, then it should be done.

it shouldn't be a problem moving the cookies part out of $browser.

I'd be happy to refactor both (assuming $cookies as a function doesn't break compatability).

Considering someone might be using the $browser (and $browser.cookies) directly, shouldn't there be a deprecation period?

@Illniyar

Just considered a backward compatability issue with $cookies as a function.
if someone bind $cookies directly (for instance using ng-repeat), then if it's a function, it will not get dirty once we change the function's keys.

how to refactor this not very modular implementation depends mostly on how it's used, and what backward compatability we can break.

@petebacondarwin

Merged in 8efadf8 feat($cookieStore): $cookieStore.get now parses blank string as blank as cf4729f and 1240641.

@Illniyar

I can get this done in a few days, but I would really like to know how you want the API (and what backward compatability issues) too look like before I start.

@Illniyar

So a month now since the last comment here.
Unless you have a better idea, I will work on getting $cookies to be a function (thereby breaking compatability with anyone using $cookies directly as a persistable hashmap).
Hopefully we can get this committed soon, I would really like to see this in (I'm using it in my own programs)

@Illniyar

I'm having a bit of trouble with running tests after I've updated the repository to the new master. Seems like Bower was added, and I'm missing something, am I supposed to run some command that will create /componenets/jquery ?

@mike-spainhower

You are probably looking for

# bower install
@Illniyar

Yeah, that was it (along with some nasty Windows only issues bower has), thanks.

I've merged the latest changes. I'll work on getting rid of $browser next.

@petebacondarwin

grunt package should run bower install automatically.
On Windows you must run as an administrator to get the symlinks to work for docs generation and e2e testing

@Illniyar

I've looked at movinng the cookies services outside $browser and getting rid of $browser entirely.
There are a few issues:

  • $browser.cookie is still in use by modules that don't rely on $cookies, specifically:
    • $http.$get uses cookie to send a xsrf token. (though we could potentially just access document.cookie directly. but that also means having no abstraction for the tests- we'll have to use the document.cookie object directly as well)
  • $browser.url (and $browser.baseHref) is used in many places. I'm not sure where to move it to, directly to angular?
    • there is some logic in the $browser's url handling, including setting up event listeners, that can be used in testing to simulate certain scenario. I'm guessing we can move this to the $location service, but I'm wondering if that'll be wise.

I can move the $cookies code out of the $browser if we choose to do so, but I'm not comfortable moving all the other functionality away from $browser (such as the scenario runner specific- notifyWhenNoOutstandingRequests).

Also I think it might be better to add the original feature first, and then refactor, it's a functionality that is surely lacking to many angular users (including me).

@eddiemonge

any news on this?

@episodeyang

Yeah this feature is definitely needed. Thanks @Illniyar and @petebacondarwin !

@philjones88

Just hit this issue too, no news yet?

@BartlomiejSkwira

We've also hit this issue when working on KarmaTracker @ Amberbit. Using jQuery.cookie I created a wrapper that overrides $cookieStore provider - it is syntax compatible with the PR. I suppose that the PR will eventually get merged - then just delete the wrapper.

#=require jquery-2.0.3.min
#=require jquery.cookie

KarmaTracker.provider '$cookieStore', ->

  this.$get = ->
    return {
      get: (name) ->
        $.cookie name

      set: (name, value, options) ->
        if options? && options.expires?
          $.cookie name, value, { expires: options.expires }
        else
          $.cookie name, value

      remove: (name) ->
        $.removeCookie name
    }
@saiko-chriskun

yep I'm having to fall back to overriding $cookieStore, as well...

@gorillamania

This is also affecting me (I need to be able to set the path and the expires. I guess I'll use jQuery Cookie until this is resolved.

@eladelrom

+1 gorillamania -- I have the same problem. Without enabling the path you can't set wild card and use the cookie on all pages of a site. Options are either putting a temporary hack or use jQuery -- both options don't seem very appealing to me.

@solidspark

+1, please merge this PR.

@KilianSSL

+1, need options

@davorpeic

:+1: yes pls

@marklagendijk

I took the example of @BartlomiejSkwira and converted into working Javascript. It uses jQuery.cookie.

/**
 * Requires jQuery and jQuery.cookie (https://github.com/carhartl/jquery-cookie)
 * - Adds an 'options' parameter to $cookieStore.put and $cookieStore.remove.
 * - Default options can be set by calling $cookieStoreProvider.setDefaultOptions
 */
myModule.provider('$cookieStore', [function(){
    var self = this;
    self.defaultOptions = {};

    self.setDefaultOptions = function(options){
        self.defaultOptions = options;
    };

    self.$get = function(){
        return {
            get: function(name){
                var jsonCookie = $.cookie(name);
                if(jsonCookie){
                    return angular.fromJson(jsonCookie);
                }
            },
            put: function(name, value, options){
                options = $.extend({}, self.defaultOptions, options);
                $.cookie(name, angular.toJson(value), options);
            },
            remove: function(name, options){
                options = $.extend({}, self.defaultOptions, options);
                $.removeCookie(name, options);
            }
        };
    };
}]);

Default options can be set as follows:

myApp.config(['$cookieStoreProvider', function($cookieStoreProvider){
    $cookieStoreProvider.setDefaultOptions({
        path: '/', // Cookies should be available on all pages
        expires: 7 // Store cookies for a week
    });
});
@shea256

@BartlomiejSkwira + @marklagendijk - thanks for the jQuery cookie wrapper. It works quite well.

@BartlomiejSkwira

@marklagendijk
My example also works, just with CoffeScript :P

@rxl
Glad I could help!

@Illniyar

so we are now in 1.2.0-RC3. What milestone is this planned to be added?

@elvenking

This feature is a must. Cookie without expiration is like half-baked cookie.

@mattdanskey

+1. Setting the expiration of a cookie is needed for me to consider using $cookieStore.

@IgorMinar
Owner

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@Illniyar

I signed it again (Electronically). Email Illniyar@gmail.com

@IgorMinar
Owner

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@Illniyar

I go to the link, I fill out the form, I press submit. I am congratulated by a pop up that informs me my CLA will be processed shortly.
What else am I supposed to do exactly?

@IgorMinar
Owner

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@BartlomiejSkwira

Here is my code for Angular 1.2.4 (and lower?)

MyNamespace.factory '$cookieStore', ->
  get: (name) ->
    $.cookie name

  set: (name, value, options) ->
    if options? && options.expires?
      $.cookie name, value, { expires: options.expires }
    else
      $.cookie name, value

  remove: (name) ->
    $.removeCookie name
@dkrugman dkrugman referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@adiherzog adiherzog referenced this pull request in scenarioo/scenarioo
Closed

Fix info popup dialog #62

@BartlomiejSkwira

@IgorMinar
Will this be merged?

@lucianenache

what is the current status of this feature and when is going to be added? I would love to contribute if there's need

@caitp
Collaborator

At a minimum, the bitrot is strong with this one, so it's hard to comment on how merge-able it is until that gets cleaned up

@Illniyar

bitrot?

@caitp
Collaborator

In need of rebase

@Illniyar

Well most of the changes are quite contained, and from a brief look it doesn't appear like there were serious changes made to $cookies or $browser since this was made, so I'm not foreseeing a difficult merge.

But without having a sense of what version it can go in, I'm not very eager to merge it only to have to remerge it again on the next version.

@caitp
Collaborator

There are merge conflicts currently, and a lot of stuff that really doesn't belong in the patch in the first place, it's asking a lot to get the person who wants to check it in to deal with the merge conflicts for you. Please be kind and rewind

@Illniyar

What stuff does not belong in the patch?

I'm not asking anyone to merge the conflicts, I'm asking to know that if I do I won't need to do it again for the next major version that passes this patch by.

@caitp
Collaborator

All of the unnecessary whitespace changes, and changes to the karma configs.

Anyways, I don't think we can promise to merge this, right now it's unmerge-able and very difficult to review. If you make that easier for us by removing unneeded stuff from the diff, that will help a lot.

@Illniyar

From what I recall, the white space changes were fixed already.
Karma config was required in order to test path specific cookies in e2e checks (or at least I wasn't able to find a different way to do it back then).

@caitp
Collaborator

No, the 2-space indentation has been replaced with 4-space indentation, which makes the unified diff much harder to review. With whitespace disabled it's not so bad, but the style does need to be fixed up.

Example/e2e tests: It's all protractor now so those would need to be updated to use the new format. Finally, all of the e2e tests run within the context of the docs app (currently), no changes are necessary to the karma config to make those work. You can see a bunch of how this looks at #6232 ... actually thats a bad example since julie already merged ptor example conversions, so lemmec... 7aef2d5 that's a better example... However once the dgeni migration happens, it will prpbably need to change more.

@kbanman kbanman referenced this pull request from a commit in kbanman/angular.js
@kbanman kbanman feat(ngCookies): More powerful $cookies service (Refs #950)
This PR allows setting additional parameters on cookies created by $cookieStore. The changes are only on $browser to avoid major changes.

It seems to me the cookie logic could be somehow moved to $cookies, but I will leave that to someone else.

The `expires` parameter can be specified as integer seconds or as a `Date` object.

The parameters are set as follows:
```
angular.extend($browser.cookieOptions, {
  path: '/',
  domain: '.example.com',
  expires: 3600
});
```

Closes #590, and is an alternative PR to #2459
0f0d6f5
@IgorMinar IgorMinar modified the milestone: 1.3.0, Backlog
@IgorMinar IgorMinar self-assigned this
@IgorMinar
Owner

I'm bumping this to 1.3.0 milestone, but I'm not making any promises. We are working on a ton of other things that got more votes from the community, so this one is definitely a low priority item at the moment.

The magnitude of the change needed makes it really hard to process this PR, but there is no way around that, it's just a matter of finding the time to do it.

@Onumis

+1 for this!

@tangentfairy

I'm having the same issue, would like to see this added

@gurdotan

Same here, this is really needed.

@jdscolam

+1 as well.

@MasFauzi

I need it

Host connection

@spythonman

+1 too..

@petebacondarwin petebacondarwin modified the milestone: 1.4.x, 1.3.x
@altryne

:+1:
I really think that using routing in your SPA, without the possibility to define that all cookies will have the same path is problematic and even error prone!

@petebacondarwin

@altryne - does #10530 solve the problem for you?

@altryne

@petebacondarwin hey, I'm not sure how to try this out, but what I don't see there is a global config, so I can set up my default params like path , expiry and domain once

@petebacondarwin

@altryne - can you comment on that PR so that @shahata can ensure that he covers your use case?

@altryne

@petebacondarwin now looking at it again, this was indeed supported!
An options parameter was added to $cookieStore.put() which supports passing path, domain, expires and secure parameters.
thanx!
hope this will land sometime soon

@lgalfaso
Collaborator

The new cookies implementation landed, this can be closed

@lgalfaso lgalfaso closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 20, 2013
  1. @Illniyar

    feat(ngCookies): support passing path/expires attributes to cookies

    Illniyar authored
    $cookieStore service had no way to set cookie attributes (such as expires).
    The api for $cookieStore.put and $cookieStore.remove now has a third argument-
    options,which allows setting cookie attributes, or deleting cookies under a
    specific path.
    - to change the path and/or expiration date of a cookie, you can pass an
      options object as the third argument to cookies(name,value).
      the options object can override Path and Expires when adding a value.
      You can pass either path, expires,none or both.
    - the path option is checked to be a string and be partial to window.location
      otherwise, the default path is used. (warning is logged)
    - the expires option is checked to be a Date and in the future,
      otherwise no expiration is set. (warning is logged)
    - you can now delete a cookie on a specific path by calling
      cookie(name,undefined,{path:x})
    
    The $cookie service remains unchanged since supporting cookie attributes
    would mean serious backward comptability issues.
    Foundations were put in place to make supporting $cookie easier.
    GENERIC TEST CHANGE:
    To support checking cookie path, all unit-tests now run on path "/karma/tests"
    Documentation was updated for both $cookie and $cookieStore with examples.
    
    closes	#1786, #1320
    BREAKING CHANGE: As part of the change, deleting a cookie  now deletes
       cookies set in multiple paths (and duplicate cookies if exists).
       Previously only cookies set in the / path were deleted.
       Since it is not intutive, if this change breaks someones code, it is
       probably as an accidental side-effect.
       It is reasonable to assume that most people actually wanted to delete the
       cookie even if it wasn't set in the same path (since they can see it).
       So while this is a breaking change, it fixes bad behaviour.
       If needed, you can delete the cookie in a specific path using $cookieStore.
  2. @Illniyar
Commits on Apr 21, 2013
  1. @Illniyar
Commits on Apr 22, 2013
  1. @Illniyar
Commits on Apr 26, 2013
  1. @Illniyar
Commits on Jun 20, 2013
  1. @Illniyar

    merge from master

    Illniyar authored
Something went wrong with that request. Please try again.