atmos / warden-github

:lock: warden strategy for github oauth
MIT License
54 stars 41 forks source link

Cache user memberships #28

Closed fphilipe closed 11 years ago

fphilipe commented 11 years ago

This caches the membership statuses in the session user object.

When using memberships for authorization it makes sense to cache these values to prevent repeated API calls.

The marshaled user object printed as YAML contains the memberships as follows:

warden.user.default.key: !ruby/struct:Warden::GitHub::User 
  attribs: 
    [...]
    member: 
      org_pub: 
        rails: false
      org: 
        rails: false
      team: 
        345: true
    [...]
  token: […]

It definitely makes sense to expire the cached values. Not really sure how though :confused: Relying on the session to timeout probably isn't a good solution. How long do you think would it make sense to keep the cached values around? Would it make sense to expire all memberships at once or every individual one? I'd say that a couple hours (6 maybe?) is long enough and expiring all of them is good enough, too.

fphilipe commented 11 years ago

Hey @atmos, I finished a first version of warden-github-rails.

My gem could really make use of membership caching. Any thoughts on it? Do you prefer to keep this out of your gem and let the dependents deal with it?

atmos commented 11 years ago

Hey @fphilipe,

I guess my concern here is that the caching would end up being used stupidly and end up giving people access to stuff that they shouldn't. Also stuff that's revoked.

As long as calls are made on behalf of the logged in user, this should all just 404. I'm a bit more concerned about access levels being used to view shit based on credentials that are old/stale.

@mastahyeti you have any thoughts on caching this stuff in the session?

fphilipe commented 11 years ago

I see your point.

What about caching for couple minutes only? Assuming you're browsing on a team restricted area and doing 5 requests a minute, I'm afraid that these API roundtrips will easily add up and be very noticeable.

I'd wager to say that caching for 5 minutes wouldn't be a problem in terms of security. Team and organization memberships aren't something volatile and thus these couple minutes would be negligible. :confused:


PS: Would you mind releasing a new version so I can release a first version of my gem? Currently I depend on the GitHub repo for the config stuff.

btoews commented 11 years ago

Well, this is all happening on the api-consumer's side, right? If their information about memberships is out of date and they try to take an action that relies on those memberships, it should just 404.

The only concern would be if their application relied on the team membership information for authorization. Then the lag between revoking team membership on GitHub and the propagation to the app would mean unauthorized access.

I don't think that app designers would expect this propagation to be instantaneous, so I don't see how a delay of a couple of minutes would hurt, but hours would probably be too much.

As for keeping it in the session, I don't see any problems.

fphilipe commented 11 years ago

So, I updated the code. It now caches the memberships during 5 minutes. Also, false memberships are not cached since this might be annoying (A: "I can't access that URL", B: "Let me give you access to it", A: "Still not working")

atmos commented 11 years ago

I'll try to get a real gem released later today or tomorrow. Thanks again for all the patches.

fphilipe commented 11 years ago

You're welcome! Glad to see it merged.