getSSOData should call /ssodata from the ULP #733
Conversation
| var url; | ||
| var params = ''; | ||
|
|
||
| if (typeof withActiveDirectories === 'function') { |
There was a problem hiding this comment.
Since this getSSOData is a new function, isn't it better to flip the parameters order instead of doing this replacement?
There was a problem hiding this comment.
it has to be the same signature as v8: https://github.com/auth0/auth0.js/blob/v8/src/authentication/index.js#L356
also, callbacks should always be the last argument
| params = | ||
| '?' + | ||
| qs.stringify({ | ||
| ldaps: 1, |
There was a problem hiding this comment.
just copied from v8: https://github.com/auth0/auth0.js/blob/v8/src/authentication/index.js#L375
There was a problem hiding this comment.
but what does passing this parameter do?
There was a problem hiding this comment.
I have no idea what the ssodata endpoint does with this param
|
|
||
| url = urljoin(this.baseOptions.rootUrl, 'user', 'ssodata', params); | ||
|
|
||
| return this.request.get(url, { noHeaders: true }).withCredentials().end(responseHandler(cb)); |
There was a problem hiding this comment.
just copied from v8: https://github.com/auth0/auth0.js/blob/v8/src/authentication/index.js#L382
There was a problem hiding this comment.
but what does passing this parameter do?
There was a problem hiding this comment.
doesn't send the headers https://github.com/auth0/auth0.js/blob/master/src/helper/request-builder.js#L79
There was a problem hiding this comment.
This is also removing the telemetry header.. What's the reason for not sending any headers now? I know you copied this from another project, but I need a reason not to add it now.
There was a problem hiding this comment.
probably because we don't want to track ssodata? I don't know.
There was a problem hiding this comment.
@cocojoe after the discussion we had today regarding tracking, is this something you want to track?
| }); | ||
| it('returns ssoData object with lastUsedConnection and idtokenpayload.name when there is no idtokenpayload.email', function( |
There was a problem hiding this comment.
idtokenpayload -> idTokenPayload
| }); | ||
| it('returns ssoData object with lastUsedConnection and idtokenpayload.email by default', function( |
There was a problem hiding this comment.
idtokenpayload -> idTokenPayload
| lastUsedClientID: '...', | ||
| sessionClients: ['...'], | ||
| sso: true | ||
| context('when inside of the hosted login page', function() { |
There was a problem hiding this comment.
Since this is a different context, please separate it with 1 or 2 new lines.
| afterEach(function() { | ||
| windowHelper.getWindow.restore(); | ||
| }); | ||
| it('calls webauth._universalLogin.getSSOData with same params', function() { |
There was a problem hiding this comment.
Please explain me this test case. Why is it handling the same parameters to the other class's method and why is it accepting strings when it should be a boolean and a function?
There was a problem hiding this comment.
in the ULP, we need to run the exact same code we were using in v8, so I'm just calling the old getSSOData (now inside the HostedPages class) with the same params. I don't really care what are the params now, just want to test that I'm just relaying the params correctly
test/web-auth/hosted-pages.test.js
Outdated
| request.get.restore(); | ||
| }); | ||
|
|
||
| it('should call ssodata with all the options', function(done) { |
There was a problem hiding this comment.
changed to /user/ssodata
test/web-auth/hosted-pages.test.js
Outdated
| done(); | ||
| }); | ||
| }); | ||
| it('should call ssodata with all the ad options', function(done) { |
There was a problem hiding this comment.
ssodata -> ssoData. Also I'd call it: "should call ssoData with all the options including activeDirectories/ad"
There was a problem hiding this comment.
I made AD uppercase
| cb: function(cb) { | ||
| cb(null, { | ||
| body: { | ||
| sso: false |
There was a problem hiding this comment.
Can you please add one more parameter to this body? Same for the test below. Why? It makes some noise to me that the sso: false is also passed on many of the tests above of this PR, and I want to fully understand what this getSSOData call ends doing.
There was a problem hiding this comment.
sso:false is simulating the server response.
There was a problem hiding this comment.
Then why is it called "with all the options" if you're just passing 1? The previous tests I mentioned have more parameters. Is this correct with a single one?
There was a problem hiding this comment.
yeah.. I just copied/pasted from v8. I renamed the test
I agree with this, but I'll do it in a different PR (replace everything related to HLP to ULP) |
luisrudge
force-pushed
the
fix-ssodata-hlp
branch
from
08e070b to
95bf9fc
Compare
currently, lock does this already, but auth0.js will always use checkSession, which is not supported in the ULP. This PR adds compatibility to the ULP when you want to call getssodata.