match endpoint on either the whole string or the part before the ?. #89

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@elmerbulthuis

I use this fix to make the library work with google apps.

I just noticed that there is already another fix for this issue available via a pull request! It would be great if you could merge either this one or the other one and publish it to npm!

happy holidays!

@elmerbulthuis elmerbulthuis match endpoint on either the whole string or the part before the ?.
I use this fix to make the library work with google apps.
cb97308
@havard
Owner
havard commented Dec 9, 2012

What I would really like to see is specific examples of what the endpoint is, and what we try to match it against, so that we could be able to find the right fix. (See my comment on the previous patch.)

The query string is part of the endpoint according to the spec. We either store the wrong endpoint, use the wrong endpoint, or get the wrong endpoint back when requesting it again.

@elmerbulthuis

heyy havard! thanks for the quick response!

I logged params['openid.op_endpoint'] and provider.endpoint in
the _verifyAssertionAgainstProvider function. Both have the same value: '
https://www.google.com/a/meet.nl/o8/ud?be=o8'

This also explains why the original code does not work. It cuts off the
part after the '?' and tries to match the strings.

I think the solution is to either cut of the part after '?' in both
endpoints, or don't cut of that part at all. I added the last solution in
my pull request...

gr-

2012/12/9 Håvard Stranden notifications@github.com

What I would really like to see is specific examples of what the endpoint
is, and what we try to match it against, so that we could be able to find
the right fix. (See my comment on the previous patch.)

The query string is part of the endpoint according to the spec. We either
store the wrong endpoint, use the wrong endpoint, or get the wrong endpoint
back when requesting it again.


Reply to this email directly or view it on GitHubhttps://github.com/havard/node-openid/pull/89#issuecomment-11175036.

@thedufer
Contributor

I would like to see this fix merged. The problem is that the query string is cut off endpoint, but not provider.endpoint, so even if they were the same before the trimming, it will fail if the endpoint has a query string. This patch tries both ways, with and without the trimming, although its not clear to me why the trimming is ever necessary.

Edit: Okay, it looks like this has been fixed - but the version hasn't been bumped, so we're not seeing the fix. I'm applying a patch equivalent to pull request #77 to fix this issue, which seems to do it.

@havard
Owner
havard commented Dec 19, 2012

@thedufer Please clarify: Is the latest version (without the suggested patch) working fine for you?

I am holding back on releasing a new version until we are clear on whether there actually is an issue, and what the issue is.

And, I am still convinced that the proper fix is not to strip query strings at all, but I need help from someone using node-openid with a Google apps domain to verify this.

@elmerbulthuis

Well, i am using your code with google apps domain, and i need it to not
strip the querystring. I am currently using my fork of your code to login
via our google apps domain.

The while reason why i made the fork is to get it to work with google apps!
:-)

gr-

2012/12/19 Håvard Stranden notifications@github.com

@thedufer https://github.com/thedufer Please clarify: Is the latest
version (without the suggested patch) working fine for you?

I am holding back on releasing a new version until we are clear on whether
there actually is an issue, and what the issue is.

And, I am still convinced that the proper fix is not to strip query
strings at all, but I need help from someone using node-openid with a
Google apps domain to verify this.


Reply to this email directly or view it on GitHubhttps://github.com/havard/node-openid/pull/89#issuecomment-11524226.

@thedufer
Contributor

I have not tried at tip, but this changeset 0b8fdf6 fixes it for me, and on inspection nothing more recent would break it again. @elmerbulthuis Try at tip rather than 0.4.2; that looks like its been fixed (and in a slightly cleaner way than your pull request).

@havard
Owner
havard commented Dec 19, 2012

OK, so here's the timeline for node-openid query string stripping:

For some reason (of which I am oblivious now) I introduced query string stripping in a98cac3 about 9 months ago.
#75 was pulled about 6 months ago, and removed query string stripping.

Not stripping is the proper way to handle this.

So, if you use the latest HEAD, are you still experiencing issues?

@elmerbulthuis

I would prefer no stripping of the qs at all. I will check the head
tomorrow.
Op 19 dec. 2012 15:58 schreef "Håvard Stranden" notifications@github.com
het volgende:

OK, so here's the timeline for node-openid query string stripping:

For some reason (of which I am oblivious now) I introduced query string
stripping in a98cac3a98cac3 9 months ago.
#75 #75 was pulled about 6
months ago, and removed query string stripping.

Not stripping is the proper way to handle this.

So, if you use the latest HEAD, are you still experiencing issues?


Reply to this email directly or view it on GitHubhttps://github.com/havard/node-openid/pull/89#issuecomment-11532753.

@elmerbulthuis

when i include the current master with our google apps domain, everything seems to be working! So this pull request is obsolete now...

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