Skip to content

Conversation

robertjd
Copy link
Member

@robertjd robertjd commented Nov 1, 2016

@typerandom please help my sanity check this one. If you agree with this quick back-compat fix, we should publish this as version 0.0.25 of this module, and update express-stormpath to use it (and release express-stormpath). This is a fix for stormpath/express-stormpath#539

The problem: we've already introduced the new "scope" property on the directory provider resource, you can see this in the REST API if you look at any of your social directories. At some point in the near future, we will instruct developers to put the desired scope on the directory provider, rather than local framework configuration. But because this is already in production, we're pulling down those scope values, and overwriting what is defined locally.

This causes two problems:

  1. Because the new remote scope values are an array, we aren't properly rendering them in google-login-form.jade. They get rendered as a comma separated string, not a space separated string.

  2. If you have defined custom scopes in your express app, via web.social.<provider>.scope, they just get overwritten by the remote config.

As such, this is a pretty big problem, because anyone who is trying to use social login is going to get a broken app next time they restart it :( so we have to fix this with a release, and put the word out that the release is the fix for this problem.

As such, I propose the fix in this PR. It will preserve whatever is defined locally. For those who are defining their own scope, we don't touch it. For those who just want the current defaults (that get loaded from lib/config.yaml), we'll preserve those too.

Then I propose a follow-up release in the future, once we officially prompt everyone to start moving scope definition to the REST API: we drop the default scopes from the default config.yaml. When we do this, we can know if the developer has specified custom scope because the local value will be coming from them, not the config.yaml. In this case we continue preserving scope, but place a warning that local scope is being used, and remote config should be used instead.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage decreased (-0.2%) to 76.786% when pulling 3807baa on social-scope-fix into 3a9e5b5 on master.

@typerandom
Copy link
Contributor

You arguments sound valid and I'm happy with the change so I'm going to merge this in.

@typerandom typerandom merged commit 824bab3 into master Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants