Ember-simple-auth: Invalidate method fails in production build for unauthenticated users

[c-emil]

When running this.get('session').invalidate() with unauthenticated user, I get this error in production build:

Error while processing route: login Cannot read property 'split' of null TypeError: Cannot read property 'split' of null

It happens because in invalidate method, it tries to look up authenticator, but the authenticator that is passed to the method is empty:

invalidate: function invalidate() {
  var _this2 = this;

  this._busy = true;
  assert('Session#invalidate requires the session to be authenticated!', this.get('isAuthenticated'));

  var authenticator = this._lookupAuthenticator(this.authenticator);
  ...
}

this.authenticator is empty because the session is not authenticated. In production build assert method is replaced with empty function, therefore it won't throw any assertion error and just continues, which causes already mentioned error.

I think it would make sense to explicitly state in docs that invalidate method should not be called on unauthenticated session or a condition should be added (keeping the assert as well) to the invalidate method that will check for whether user is authenticated and if not, don't continue. What do you think?

[marcoow]

I think it would make sense to explicitly state in docs that invalidate method should not be called on unauthenticated session

Yeah, that sounds like a good idea. PR would be welcome!

[mirague]

We experience this on production as well and hadn't dived into the issue myself. Happy to see a write-up with explanation posted here – saved us some time!

Will add a manual check for isAuthenticated before calling invalidate()

[BryanCrotaz]

It would make it easier for developers if invalidate just returned immediately if !isAuthenticated

[marcoow]

It would make it easier for developers if invalidate just returned immediately if !isAuthenticated

I strongly believe this would make the API much harder to understand and use - returning a promise in one scenario and returning immediately in another scenario means you have to check which scenario you're in before you can even call the method as you have to handle the returned value totally different in the 2 scenarios.

[BryanCrotaz]

... so return an already resolved promise...

after invalidate(), the session should be invalidated. So if it's already invalidated, then the result of invalidate() should be the same as if it caused it to invalidate.

[marcoow]

Yeah, I agree removing the assertion might be a good idea. I guess removing an assertion should also not be considered a breaking change?

[marcoow]

This is a good first issue as the only thing that needs to be done is to remove the assertion (and potentially a test that asserts the assertion exists :) ).