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

Configurable server post object properties #346

Closed
wants to merge 1 commit into from
Closed

Configurable server post object properties #346

wants to merge 1 commit into from

Conversation

knivets
Copy link

@knivets knivets commented Apr 8, 2015

It is not very flexible to have them hardcoded, especially if you have to adapt to the backend you have no control over. I'm talking about the object with code, clientId and redirectUri fields. I made a quick tweak, that allows to override default field names for custom scenarios and fall back to default values if not necessary, so the tweak doesn't break anything. I'm not sure if tests are needed for such a small tweak. But anyway, let me know. Thanks.

@philippeluickx
Copy link
Contributor

+1

@sahat
Copy link
Owner

sahat commented Aug 15, 2015

I don't like that it's a global config, not to mention it's very specific to OAuth 2.0 and therefore shouldn't called authFields. Perhaps it's best to allow overriding defaultUrlParams at each individual provider level? You can already do that.

    google: {
          name: 'google',
          url: '/auth/google',
          authorizationEndpoint: 'https://accounts.google.com/o/oauth2/auth',
          redirectUri: window.location.origin || window.location.protocol + '//' + window.location.host,
          scope: ['profile', 'email'],
          scopePrefix: 'openid',
          scopeDelimiter: ' ',
          requiredUrlParams: ['scope'],
          optionalUrlParams: ['display'],
          // will override OAuth 2.0 defaults...
          defaultUrlParams: ['response_type', 'client_id', 'redirect_uri', 'foo', 'bar'],
          display: 'popup',
          type: '2.0',
          popupOptions: { width: 452, height: 633 }
        },

For example, if you override this property, it will use your fields, if not, it will fallback to ['response_type', 'client_id', 'redirect_uri'].

@sahat sahat added the 0.12 label Aug 15, 2015
@sahat
Copy link
Owner

sahat commented Aug 15, 2015

Ah, I am sorry I misunderstood your intent there. If you want to send additional params in exchangeForToken I suppose it makes sense to make it customizable. I'll see what I can do.

Update

Somewhat related issue: #237.

@sahat
Copy link
Owner

sahat commented Aug 15, 2015

Ok, I was able to kill 2 birds with 1 stone. Instead of creating new fields, I have slightly changed how the existing responseParams property works. Instead of an array, it is now an object.

Here are OAuth 2.0 defaults:

var defaults = {
  responseParams: {
    code: 'code',
    clientId: 'clientId',
    redirectUri: 'redirectUri'
  },
  defaultUrlParams: ['response_type', 'client_id', 'redirect_uri'],
  responseType: 'code'
};

And here is how you can change param names, as well as add a new response field, given that your OAuth 2.0 provider is sending additional fields besides code. For example, let's assume Facebook returns domain_prefix in addition to code. Here is a sample config using the new responseParams:

facebook: {
  name: 'facebook',
  url: '/auth/facebook',
  authorizationEndpoint: 'https://www.facebook.com/v2.3/dialog/oauth',
  redirectUri: (window.location.origin || window.location.protocol + '//' + window.location.host) + '/',
  scope: ['email'],
  scopeDelimiter: ',',
  responseParams: {
    code: 'code',
    clientId: 'client_id',
    redirectUri: 'redirect_uri',
    domainPrefix: 'domain_prefix'
  },
  requiredUrlParams: ['nonce','display', 'scope'],
  display: 'popup',
  type: '2.0',
  popupOptions: { width: 580, height: 400 }
},

Your server will receive the following object:

{ client_id: "....", code: "....", redirect_uri: "....", domain_prefix: "..." }

@philippeluickx
Copy link
Contributor

Will this also work with a nested response? e.g. http://jsonapi.org/
{ data: { attributes: { code: xyz, ...} } }

@sahat
Copy link
Owner

sahat commented Aug 15, 2015

@philippeluickx It iterates once, so just the top-level properties.

@sahat
Copy link
Owner

sahat commented Aug 15, 2015

@philippeluickx However, userData can be a nested object, which will be sent to your server along with code, client id, redirect uri:

$auth.authenticate = function(name, userData) {
     return oauth.authenticate(name, userData);
};

@sahat sahat closed this in 15e1d56 Aug 15, 2015
@sahat sahat reopened this Aug 15, 2015
@philippeluickx
Copy link
Contributor

Yup, userData can be nested, have done that already so I can confirm here.
If the response that you get can be configurable, than the backend can stick to a certain specification and is not dictated by the frontend.
Not a dealbreaker for me at the moment, but this then requires slightly less elegant approach.

@sahat
Copy link
Owner

sahat commented Aug 16, 2015

Resolved inResolved in 0.12 release.

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

Successfully merging this pull request may close these issues.

3 participants