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

Remove the google `profile` scope. #6975

Merged
merged 2 commits into from Aug 2, 2016

Conversation

Projects
None yet
4 participants
@MasterAM
Contributor

MasterAM commented May 5, 2016

This is a re-submission of #5699.

The email scope is enough for getting the required data, including the user ID, so no need for the profile scope, which may deter users.

The discussion started at #5650

Our team anticipates Google to be a major choice for users during signup, so we would like to alleviate some of the concerns that requesting the profile scope may bring.

We would also like developers to have more control over more aspects, such as the way the popup window behaves (e.g, forcing it upon login for situations where there is more than one logged-in Google account).

I would really like to hear your opinions in the discussion thread.

@stubailo stubailo self-assigned this May 24, 2016

@stubailo

This comment has been minimized.

Contributor

stubailo commented May 24, 2016

I'm a bit worried this might be a breaking change for people who are currently using the profile information in their apps. Perhaps there is something else we could do to allow one to disable the profile scope?

@stubailo stubailo removed their assignment May 24, 2016

@MasterAM

This comment has been minimized.

Contributor

MasterAM commented May 24, 2016

@stubailo, We could make it configurable via Meteor.settings, and have it add the profile scope by default.
Having an explicit setting could override it.

Having said that, there are a couple more things I would like to make configurable about the Google auth process. Would you advise to add them to this PR or create a separate one once this one is agreed upon and merged?

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Jun 28, 2016

@MasterAM, sorry about the delays here.

A couple of things:

  1. I think we should probably just swap the requiredScope and scope so by default the behavior is the same, but it is now possible to remove the profile scope by passing requestPermissions = [].
  2. I think we should remove locale from here: https://github.com/meteor/meteor/blob/devel/packages/google/google_server.js#L4-L5
  3. We need a (prominent) changelog entry.

@tmeasday tmeasday self-assigned this Jun 28, 2016

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Jul 19, 2016

@MasterAM, thanks again for this PR. We're still waiting to hear about the comments above--just to let you know, we'll be closing this PR in a week if we don't.

@MasterAM

This comment has been minimized.

Contributor

MasterAM commented Jul 21, 2016

Hi Tom,

Forgot about the PR, thanks for reminding me.

  1. No problem. Another option would be deprecating the current API (providing a deprecation message) and introducing a new config options (requiredPermissions or extraPermissions or any other suitable name) that will use the email scope by default, but that might be too much of a hassle.

    A major version bump may be in place.

  2. I don't see why. If a user wants a scope that returns locale (if available), why won't it be whitelisted? Do you expect them to directly mutate the
    When the current Google.whitelistedFields list?

  3. No problem. Do you want me to write one? If so, where?

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Jul 25, 2016

  1. I'd advocate the easiest thing :)
  2. I mean it just seems weird to have a whitelisted field that we don't know is there for sure. But we could also just leave it I guess.
  3. History.md

Thanks again!

Also, can you merge devel (or rebase) into this branch so the tests pass? Thanks!

MasterAM added some commits Nov 23, 2015

Removed the google `profile` scope.
The `email` scope is enough for getting the required data, including the
user ID.
Improve backward compatibility and add note
The default value for the `requestPermissions` option is now `profile`,
thus preserving the old behavior in case the option was not
explicitly set.

Added a note in the History.md file regarding this change.

@MasterAM MasterAM force-pushed the MasterAM:feature/google-permissions branch from 9939e16 to ca09e9d Jul 27, 2016

@MasterAM

This comment has been minimized.

Contributor

MasterAM commented Jul 27, 2016

Rebased, made changes and added the changelog entry. I Hope it's not too verbose.

Note to users:

  • If you did not specify the requestPermissions option, you don't have to change anything.
  • If you specified this option and want to request the profile permission, you should add it to the array of your requested permissions.
  • If you do not need the profile scope, it will not be used if you pass a (potentially empty) array that does not include it as the requestPermissions option.

If unsure, take a look at the code, it's really simple.

@tmeasday tmeasday merged commit ca09e9d into meteor:devel Aug 2, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Aug 2, 2016

Thanks very much for your hard work on this @MasterAM

@karloespiritu

This comment has been minimized.

karloespiritu commented on History.md in ca09e9d Aug 8, 2016

u

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment