Code review comment for lp:~fgallina/rnr-server/ssov2

Revision history for this message
Matias Bordese (matiasb) wrote :

Added a couple of minor comments.

Also attaching chat about verified accounts and getting SSO account details on auth (conclusion: you are good with your updates in the MP).

(16:20:55) matiasb: beuno: so, a couple of questions re RnR; should we allow unverified users to rate/review? I guess we should? and also, originally, RnR keeps user details (full name, email) updated from SSO... should that be preserved in the migration? (it requires 2 SSO API calls for each RnR API request)
(16:37:55) beuno: matiasb, I think we shouldn't
(16:38:05) beuno: it's what would be most prone to abuse, no?
(16:38:21) beuno: as for keeping user details
(16:38:23) matiasb: yeah, that's right
(16:38:27) beuno: not sure, why would it need to?
(16:39:16) matiasb: to keep the user's details up to date according to sso possible changes
(16:39:40) matiasb: that's what it is doing right now, but I wouldn't
(16:39:55) matiasb: but checking, since fabian is working on that in the MP above
(16:40:33) beuno: matiasb, so it doesn't have to call back out to SSO for them?
(16:41:13) beuno: I sort of feel we should always just call out to SSO, with some level of caching on the clients and server
(16:41:15) matiasb: beuno: it calls back SSO when creating the user, but now it is calling SSO when validating the request and then to get updated account details
(16:41:51) matiasb: beuno: right, with some delta, but I guess no every time
(16:42:10) beuno: matiasb, yeah, but I think it would be a memcache-y thing
(16:42:15) beuno: rather than a DB thing
(16:42:30) beuno: and make sure each service deals with not having that data in a reasonable way
(16:42:39) matiasb: makes sense
(16:42:45) beuno: so have the service cache that data for, say, 30 minutes
(16:42:55) beuno: and SSO cache it until it's invalidated or something like that
(16:43:03) beuno: so it's mostly memcaches talking to memcaches
(16:43:07) matiasb: heh
(16:43:19) beuno: EVERYTHING IN RAM!
(16:43:30) beuno: like the SCA testsuite
(16:43:41) matiasb: heh :-/
(16:44:01) matiasb: agree, sounds good to me
(16:44:15) matiasb: we should get that in place too then
(16:44:50) beuno: matiasb, I may regrest suggesting this
(16:44:54) beuno: but until we do
(16:45:03) beuno: I think we just ask SSO for those details each time
(16:45:14) matiasb: ack, got it

« Back to merge proposal