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

discovery-cache #34

Closed
qzaidi opened this issue Feb 19, 2016 · 3 comments
Closed

discovery-cache #34

qzaidi opened this issue Feb 19, 2016 · 3 comments

Comments

@qzaidi
Copy link

qzaidi commented Feb 19, 2016

Hi,

Can you explain how to use the discovery cache. I see that the cache is not at all used, because while there is a Get in verify.go, there is no Put in the code anywhere. Am I supposed to do the Put myself?

In verify.go - control goes to and discovered is always nil
168 discovered := cache.Get(endpoint)
Then it returns from this block, but I don't see cache.Put anywhere in code.
197 if ep == endpoint {
Are we supposed to Put the discovered url at this point, or do this externally? An example will help greatly.
I am sorry if the question is unclear, and i haven't fully understood the code or protocol, so it may be my flawed understanding, apologies in advance.

@xStrom
Copy link
Contributor

xStrom commented Feb 19, 2016

From what I've gathered the Put call is currently missing from the library code. I haven't had the time yet to analyse the spec properly in order to submit a pull request. However it's on my todo list and I hope to get this done sometime during this week.

So you can probably use the library under the assumption that the discovery cache is updated by the library.

@yohcop
Copy link
Owner

yohcop commented Feb 20, 2016

Hi,
Yeah, the specs aren't really clear as to when put things in the cache, but they say something like "if [this] has not been previously discovered", implying the use of a cache.

Like xStorm said, you can use it assuming the library is updating it. When this is fixed, the library should update the cache for you.

yohcop added a commit that referenced this issue Mar 4, 2016
Properly utilize the discovery cache.

Currently the discovery cache is effectively unused, as also pointed out by #34. This patch resolves this issue.

The [OpenID 2.0 spec point 11.2](http://openid.net/specs/openid-authentication-2_0.html#verify_disco) softly implies the use of a cache. However there really isn't any explicit explanation on what to cache and how.

I read through most of the spec for what must be the 10th time by now. Based on my understanding of the overall spec, the code, and tests with actual OpenID providers, I have implemented a discovery cache usage that should be correct.
@yohcop
Copy link
Owner

yohcop commented Mar 4, 2016

As you can see @xStrom implemented this now, so closing this issue. Thanks for pointing that out!

@yohcop yohcop closed this as completed Mar 4, 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

No branches or pull requests

3 participants