Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESA 1.0 #602

Merged
merged 311 commits into from Oct 16, 2015
Merged

ESA 1.0 #602

merged 311 commits into from Oct 16, 2015

Conversation

@marcoow
Copy link
Member

@marcoow marcoow commented Aug 5, 2015

Build Status

(this was previously #522 and the ember-cli-addon branch but as 1.0 will actually include more changes than just making ESA an Ember CLI Addon-only project the branch was renamed to jj-abrams following a nice ember tradition)

This will be the 1.0 release of Ember Simple Auth, turning the library into a first class Ember CLI addon and removing the bower and globals distributions. The 1.0 version will of course also support Ember 2.0.

For more info on Ember Simple Auth 1.0 see the talk @marcoow gave at the September Ember.js Munich Meetup.

Installation Instructions

  • remove the ember-simple-auth package from bower.json
  • remove and ember-cli-simple-auth* packages from package.json
  • add the new Ember CLI Addon to package.json: "ember-simple-auth": "simplabs/ember-simple-auth#jj-abrams"
  • run bower install && npm install

See the dummy app for example usage of 1.0 ESA which contains quite a few breaking changes.

I gave a talk introducing Ember Simple Auth 1.0 and the changes that come with it at Ember.js Munich: https://www.youtube.com/watch?v=aeH_9YGgt9E

TODO

  • set up the configured session store as session-stores:application which can be overridden in app/session-stores/application.js
  • fix README
  • go to the app's root url instead of simply reloading in ApplicationRouteMixin.sessionInvalidated
  • Facebook auth restoration doesn't seem to work in dummy app
  • make sure ESA registers as a library and thus shows up in the initial startup log
  • make sure acceptance tests work nice
  • turn the session into a service (actually there's a service now that wraps the session which itself isn't directly exposed anymore)
  • split the initialization code into initializers and instance initializers
    - [ ] clean up code and tests (e.g. session code is pretty messy, some tests use session instead of mocked session service etc.) (this can actually be moved to after the 1.0 release as it won't affect the public API)
  • make sure the documentation is correctly generated and convert all examples and references to Ember CLI/ES6 modules syntax
  • fix tests (currently fail in beta and canary because of emberjs/ember-mocha#48- this is fine now but the release tests fails as they seem to run with Ember 2.0 and Ember Data 1.3)
  • defining the session store in session-stores/application.js doesn't really make sense as the store isn't really extensible anyway and you'd always want to use the ephemeral store in the test env.
  • session should log if and why restoration fails
  • figure out how to handle authorizers - best option is probably to remove them completely and leave it up to the application to use the session data to inject headers into requests - that way it would also work with e.g. the ember data adapter directly, with fetch api etc. Instead of providing ready-to-use authorizers the library could provide convenience mixing for Ember Data adapters etc. that make it easy to inject headers into AJAX calls etc.
    Idea for the new API is to add an authorize method to the session that generates authorization data with a given authorizer, e.g.
this.get('session').authorize('authorizer:oauth2', (header) => {
  xhr.setRequestHeader(header.name, header.value);
});

When the session is not authenticated or there is not token, the authorizer would simply not call the block. That way the authorizer doesn't depend on e.g. jQuery anymore.

There should also be mixins for common scenarios, e.g. Ember Data:

export default DS.JSONAPIAdapter.extend({
  session: service('session'),

  ajaxOptions() {
    let hash = this._super(...arguments);
    let { beforeSend } = hash;
    hash.beforeSend = (xhr) => {
      this.get('session').authorize('authorizer:oauth2', (header) => {
        xhr.setRequestHeader(header.name, header.value);
      });
      if (beforeSend) {
        beforeSend(xhr);
      }
    };
    return hash;
  },

  handleResponse(status) {
    if (status === 401) {
      this.get('session').invalidate();
      return true;
    } else {
      return this._super(...arguments);
    }
  }
});

- [ ] move the setup of $.ajaxPrefilter into a dedicated initializer that can optionally be loaded (there should probably also be an initializer that e.g. sets up authorization on the Ember Data adapter instead). The current method of automatically loading it should be extracted into an addon so it's easy to upgrade projects in a non-breaking way. (this shouldn't actually be necessary)
- [ ] rename Authenticator.restore to Authenticator.validate (don't think this is actually a good idea)

@marcoow marcoow added the enhancement label Aug 5, 2015
@marcoow marcoow self-assigned this Aug 5, 2015
@marcoow marcoow added this to the 1.0 milestone Aug 5, 2015
@marcoow marcoow mentioned this pull request Aug 5, 2015
2 of 5 tasks complete
@marcoow marcoow force-pushed the jj-abrams branch from 4eacd1d to ac96b5a Aug 5, 2015
@peec
Copy link

@peec peec commented Aug 5, 2015

Great job on this, looking forward to it 👍

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 5, 2015

@peec: please go ahead and try it - early feedback is really important at this stage.

@felixkiss
Copy link

@felixkiss felixkiss commented Aug 6, 2015

I am trying to get this to work with one of my projects. Since there is no more ember-cli-simple-auth-testing, what is it replaced by?

In my tests/helper/start-app.js I have:

import Ember from 'ember';
import Application from '../../app';
import Router from '../../router';
import config from '../../config/environment';
import 'simple-auth-testing/test-helpers';

export default function startApp(attrs) {
  var application;

  var attributes = Ember.merge({}, config.APP);
  attributes = Ember.merge(attributes, attrs); // use defaults, but you can override;

  Ember.run(function() {
    application = Application.create(attributes);
    application.setupForTesting();
    application.injectTestHelpers();
  });

  return application;
}

But now, obviously, the tests error:

Error: Could not find module `simple-auth-testing/test-helpers` imported from `tests/helpers/start-app`

What should I import instead?

@lan0
Copy link
Contributor

@lan0 lan0 commented Aug 6, 2015

I can't comment on 5eed942 because it crashes my browser, but have you considered transitioning to ember-cli-build.js? See TRANSITION.md of ember-cli.

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 7, 2015

The test helpers haven't been transitioned to the new structure yet. I guess they would have to be moved to the test-helpers directory or so.

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 7, 2015

@lan0: transitioning to ember-cli-build.js sounds like a good idea - want to submit a pull request?

@andrewbranch
Copy link

@andrewbranch andrewbranch commented Aug 10, 2015

I’m getting Uncaught Error: Attempting to inject an unknown injection: 'session-store:ephemeral' at this stage. I’ve been using the default localStorage store, and tried specifying it explicitly, but still my app crashes with this error on load.

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 11, 2015

@andrewbranch: yeah, I think the store isn't currently registered. As a workaround it should work though when you define an application store as in the dummy app.

@marcoow marcoow force-pushed the jj-abrams branch from 5e707b6 to 7b9c03d Aug 11, 2015
@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 11, 2015

@andrewbranch: 46a24be fixes the ephemeral session store default; be aware that the ´store´ config setting has been removed though - you can use a different store by simply defining app/session-stores/application.js and return a subclass of e.g. the localStorage session store from that (that's more or less the same mechanism that Ember Data uses for its adapters).

@OpakAlex
Copy link

@OpakAlex OpakAlex commented Aug 13, 2015

@marcoow when you will merge this?
Also, what about Ember 2.0 support?

@marcoow marcoow mentioned this pull request Aug 13, 2015
@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 13, 2015

This also includes support for Ember 2.0. For now please install from GitHub - there's no ETA for the 1.0 release really - I hope soon but there are open issues still. If you want to help submit a PR!

@ivanvanderbyl
Copy link
Contributor

@ivanvanderbyl ivanvanderbyl commented Aug 13, 2015

@marcoow this is fantastic. Thanks for all your hard work.

@SeyZ
Copy link

@SeyZ SeyZ commented Aug 13, 2015

Would be great if we can have the lookup deprecation warning solved in another branch.
The only library that prevent me to upgrade to Ember 2.0.0 is ember-simple-auth.

Any chance?

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 13, 2015

@SeyZ: you can already upgrade to 2.0 and install ESA from this branch - see the dummy app in the tests for guidance.

@mattma
Copy link

@mattma mattma commented Aug 13, 2015

@marcoow Found the dummy app. I did not see the installation instruction. It would be great to include here for clarification.

// bower.json
...
    "ember-simple-auth": "~0.8.0",
    //  "ember-simple-auth": "https://github.com/simplabs/ember-simple-auth/tree/jj-abrams",  ???
...

Another question, I am going to use simple-auth with ember@2.0.0, the issue is the deprecation exception on lookup, with this fix won't change the functionalities or how it works in simple-auth, just to satisfied the ember@2.0.0 deprecation related issues, right?

@mattma
Copy link

@mattma mattma commented Aug 13, 2015

Tried with this setting

    "ember-simple-auth": "git@github.com:simplabs/ember-simple-auth.git#jj-abrams",

bower_components/ember-simple-auth/simple-auth.amd.js did not match any files

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 13, 2015

@mattma, @SeyZ: added installation instructions for this branch in the merge request description above.

@mattma
Copy link

@mattma mattma commented Aug 13, 2015

@marcoow Tried installation instruction. It works. Now i am using the #1.0.0 version. Confirmed that it got rid of warning message of lookup.

I think in the guide is also missing

add the new Ember CLI Addon to bower.json: "ember-simple-auth": "simplabs/ember-simple-auth#jj-abrams"

I got an loader error when I start my app (tried with add bower.json dependency)

Uncaught Error: Could not find module `simple-auth/authenticators/base` imported from `xx/initializers/authentication`

I have a file

// initializers/authentication.js
import Base from 'simple-auth/authenticators/base';

let LocalAuthenticator = Base.extend({
  authenticate (credentials) {
     ...
  },

  invalidate (credentials) {
   ...
  },

  restore (credentials) {
    ...
  }
});

export default {
  name: 'authentication',
  before: 'simple-auth',
  initialize: function (container, app) {
    app.register(config.simpleAuthStrategy.local, LocalAuthenticator);
  }
};

@marcoow
Copy link
Member Author

@marcoow marcoow commented Aug 13, 2015

@mattma: you should not add anything to bower.json - only add to package.json as described above. Also module names etc. have changed - see the dummy app for example usage of the new 1.0 addon. Documentation will be available soon but implementation needs to be completed first.

@mattma
Copy link

@mattma mattma commented Aug 13, 2015

@marcoow

I have removed dependency from bower.json. Look again on dummy app.

First, I have rename import Base from 'simple-auth/authenticators/base'; to import Base from 'ember-simple-auth/authenticators/base';. it got rid of all the exeception.

Then I run into Error while processing route: index Cannot read property 'isAuthenticated' of undefined TypeError: Cannot read property 'isAuthenticated' of undefined. I guess that several things have changed internally so that the behavior is changed when I use the session.

I guess I have to wait until documentation is out and do the full re-write.

@marcoow marcoow deleted the jj-abrams branch Oct 16, 2015
@rmachielse
Copy link
Contributor

@rmachielse rmachielse commented Oct 16, 2015

Congratulations!

@marcoow
Copy link
Member Author

@marcoow marcoow commented Oct 16, 2015

🎉

@rinti
Copy link

@rinti rinti commented Oct 16, 2015

🎉 thanks for the fantastic work @marcoow (and contributors)

@dustinfarris
Copy link

@dustinfarris dustinfarris commented Oct 16, 2015

Bravo! Excellent work!

@mgharbik
Copy link

@mgharbik mgharbik commented Oct 16, 2015

Awesome job! Looking forward to contribute in this project. Inspiring!

@joelcox
Copy link

@joelcox joelcox commented Oct 16, 2015

Great work @marcoow and other contributors. Thanks for this great addon.

@quaertym
Copy link
Contributor

@quaertym quaertym commented Oct 16, 2015

🎆 Thanks for the great work.

@ballPointPenguin
Copy link

@ballPointPenguin ballPointPenguin commented Oct 16, 2015

🐹 🎈 🍄

@thijsvdanker
Copy link

@thijsvdanker thijsvdanker commented Oct 16, 2015

👍 thanks for all the effort!

@orf
Copy link

@orf orf commented Oct 16, 2015

Thank you for your work on the project! This is awesome :)
On 16 Oct 2015 14:19, "Thijs van den Anker" notifications@github.com
wrote:

[image: 👍] thanks for all the effort!


Reply to this email directly or view it on GitHub
#602 (comment)
.

@constantm
Copy link
Contributor

@constantm constantm commented Oct 16, 2015

Haha! I just installed Simple Auth in my project today and I'm busy implementing a custom session with Ember Data. Time to start from scratch. 😄

@marcoow
Copy link
Member Author

@marcoow marcoow commented Oct 16, 2015

@constantm sorry for that ;)

@ryanlitalien
Copy link

@ryanlitalien ryanlitalien commented Oct 16, 2015

Thanks @marcoow, congrats! 🍰

@youroff
Copy link

@youroff youroff commented Oct 16, 2015

Awesome work @marcoow! 👍👍👍

@jcbvm
Copy link
Contributor

@jcbvm jcbvm commented Oct 16, 2015

Thanks for the great work! Happily running it on my production app

@Asherlc
Copy link

@Asherlc Asherlc commented Oct 16, 2015

Thanks for the hard work! It looks like the crossOriginWhitelist option has been removed in 1.0. Is there an equivalent to prevent CORS errors?

@Keeo
Copy link

@Keeo Keeo commented Oct 16, 2015

Good job, its nice to see its finally here.

@marcoow
Copy link
Member Author

@marcoow marcoow commented Oct 16, 2015

Auto-authorization has been dropped altogether so there's no need for a crossOriginWhitelist anymore.

Am 16.10.2015 um 20:28 schrieb Asher Cohen notifications@github.com:

Thanks for the hard work! It looks like the crossOriginWhitelist option has been removed in 1.0. Is there an equivalent to prevent CSRF errors?


Reply to this email directly or view it on GitHub.

@dukex
Copy link
Contributor

@dukex dukex commented Oct 16, 2015

Thanks all involved!!! Nice work @marcoow

@danielalbertjeffery
Copy link

@danielalbertjeffery danielalbertjeffery commented Oct 17, 2015

Docs are awesome :)

Just noticed a spelling error ...

"
OAuth2BearerAuhtorizer"

Should be OAuth2BearerAuthorizer - is in three places!

On Sat, Oct 17, 2015 at 5:18 AM, Duke notifications@github.com wrote:

Thanks all involved!!! Nice work @marcoow https://github.com/marcoow


Reply to this email directly or view it on GitHub
#602 (comment)
.

@dukex
Copy link
Contributor

@dukex dukex commented Oct 17, 2015

Thank you @danielalbertjeffery the pull request #723 fix one OAuth2BearerAuhtorizer mistake, do you can point the other mistakes you found, I think @MattNguyen can fix them

@danielalbertjeffery
Copy link

@danielalbertjeffery danielalbertjeffery commented Oct 17, 2015

Sure!

On Sat, Oct 17, 2015 at 9:26 AM, Duke notifications@github.com wrote:

Thank you @danielalbertjeffery https://github.com/danielalbertjeffery
the pull request #723
#723 fix one
OAuth2BearerAuhtorizer mistake, do you can point the other mistakes you
found, I think @MattNguyen https://github.com/MattNguyen can fix them


Reply to this email directly or view it on GitHub
#602 (comment)
.

@akatov
Copy link
Contributor

@akatov akatov commented Oct 20, 2015

👍

@XrXr
Copy link

@XrXr XrXr commented Oct 22, 2015

@NullVoxPopuli did you end up solving that issue with _actions? I am running into the exact same issue

nvm, I solved the issue. It was caused by an outdated ember-cli-document-title.

derrickshowers added a commit to derrickshowers/ember-simple-auth-parse that referenced this pull request Dec 21, 2015
The latest version of simple auth (that supports Ember 2.0) changed its
path. Instead of 'simple-auth,' it's now 'ember-simple-auth'. See
documentation here:

simplabs/ember-simple-auth#602
@ryanlabouve
Copy link

@ryanlabouve ryanlabouve commented Apr 14, 2016

@AlfaroLore I believe the jj-abrams branch has since been merged to master, so the link your wanting is likely https://github.com/simplabs/ember-simple-auth/tree/master/tests/dummy

@jdurand

This comment has been minimized.

Copy link

@jdurand jdurand commented on 2322b8e Apr 19, 2016

This breaks my build:
Error: EEXIST: file already exists, symlink '/tmp/build_[...]/node_modules/ember-simple-auth/app/routes/application.js' -> '/tmp/build_[...]/tmp/ember_script_filter-tmp_dest_dir-fiPaGEso.tmp/dashboard/routes/application.js
Any reason why you needed to make sure the application route exists?

This comment has been minimized.

Copy link
Member Author

@marcoow marcoow replied Apr 19, 2016

I assume you're using CoffeeScript or TypeScript or so?

This comment has been minimized.

Copy link

@jdurand jdurand replied Apr 19, 2016

Yes, I'm moving away from EmberScript, but I still need it for now.
I'd settle for a workaround that would avoid me the pain of maintaining a fork just for removing this file.

This comment has been minimized.

Copy link
Member Author

@marcoow marcoow replied Apr 19, 2016

That's a known limitation when using languages other than JS. Convert your application route file to JS as a workaround.

This comment has been minimized.

Copy link

@jdurand jdurand replied Apr 19, 2016

I just refactored a whole app form ember-rails & global var to ember-cli & es6 modules, I've had enough refactoring for a few weeks :-)
For my personal knowledge, what's this file used for?
Tests still pass without it.

This comment has been minimized.

Copy link
Member Author

@marcoow marcoow replied Apr 19, 2016

Ember Simple Auth must be sure that an application route exists to this works: https://github.com/simplabs/ember-simple-auth/blob/master/addon/instance-initializers/setup-session-restoration.js

Converting your application route file to JS should be pretty straight forward actually - it's really only that one file app/routes/application.js.

This comment has been minimized.

Copy link

@jdurand jdurand replied Apr 19, 2016

Converting this mess of a route would have made it even less maintainable, refactored to a temporary horrible-application-route-mess-mixin.em instead.
Thank you for your time.

@woohoou
Copy link

@woohoou woohoou commented Jun 19, 2016

Exactly same here, works converting to js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.