feat(userInfo): implement persisting user info to support limited tokens #24729
Conversation
Changed Packages
|
| @@ -190,7 +189,7 @@ export class TokenFactory implements TokenIssuer { | |||
| alg: key.alg, | |||
| kid: key.kid, | |||
| }, | |||
| payload: { sub, ent, iat, exp }, | |||
There was a problem hiding this comment.
Did we want to remove ent from all tokens now that the user info service is flushed out or just from limited ones?
There was a problem hiding this comment.
The goal is to remove it from all tokens. But we may want to keep it in there still for some time, while the userInfo usage ramps up. As long as plugins exist that look at the claim directly (via an old IdentityService for example), the data may need to stay. It's fine to make the deprecation of that field a separate effort from implementing the endpoint fully.
| .comment('User entity reference'); | ||
|
|
||
| table | ||
| .text('ownership_entity_refs', 'longtext') |
There was a problem hiding this comment.
An open question here (maybe @Rugvip) - should we make this one just user_info (with all json stringified info) and not storing all possible pieces of data one by one? Not sure that the service really will care about the detailed contents.
For context, we were discussing before about what to do with custom claims - why not store custom claims here too? Then where to put those?
There was a problem hiding this comment.
Yep, I think we will want to store custom claims here too and make this an opaque user info blob.
Signed-off-by: Phil Kuang <pkuang@factset.com>
|
Any rough estimation on when this PR will be merged and part of release as it is affecting all our users (such as leaders, tech leads etc.) who are part of many ad groups? |
| if ( | ||
| !Array.isArray(ownershipEntityRefs) || | ||
| ownershipEntityRefs.some(ref => typeof ref !== 'string') | ||
| Array.isArray(ownershipEntityRefs) && |
There was a problem hiding this comment.
I think I'd rather keep the check so effectively like
let ownershipEntityRefs = ent;
if (!ownershipEntityRefs) {
ownershipEnttityRefs = await ...;
}
if (!ownershipEntityRefs) {
throw ...
} else if (isNotValid) {
throw ...
}|
|
||
| res.json({ | ||
| sub: userInfo.userEntityRef, | ||
| ent: userInfo.ownershipEntityRefs, |
There was a problem hiding this comment.
Regarding what's stored, and what's returned over the wire: Maybe it would simplify things if the storage model was basically { claims: JsonObject }, so that what you find in the json column would typically be like {"claims":{"sub":"user:default/freben","ent":[]}} or so. At that point, maybe the returned body could have a claims root key too? 🤔
Maybe @Rugvip has opinions here, but I'm essentially wondering if who, if anyone, should do the translations back and forth between the long-form and short-form of each name an if it's at all worth it. Especially if we then open up for people to customize what set of claims that actually get persisted, so we can get extra stuff in here that come out of the sign in resolver. And with that claims key, we could also add the profile key next to it, with what came out of the profile transform during sign in.
So - what should the ideal response object shape be here?
| user_info: string; | ||
| }; | ||
|
|
||
| // TODO: How do we prune stale users? |
There was a problem hiding this comment.
These should probably accept (and store) effectively the exp of the token so that they can be cleaned up automatically (after a grace period) after the token itself becomes useless. Remember, you have to present an actual valid non-expired token to be able to read out of this anyway.
| // issuance so that it can be retrieved later by limited user tokens | ||
| await this.userInfoDatabaseHandler.addUserInfo({ | ||
| userEntityRef: sub, | ||
| ownershipEntityRefs: ent, |
There was a problem hiding this comment.
So related to my comment elsewhere - i wonder if this can't just be ({ claims }) instead. Well, we may perhaps denylist some known unnecessary fields, like exp, iat, iss, aud and uip, from the body itself, but keep whatever custom stuff remains after that in addition to sub and ent.
And this is just the internal storage model; there's nothing stopping us from translating that to something else in the JSON response from the service itself.
Hey, I just made a Pull Request!
Part of BEP-0003
Fixes #24420
✔️ Checklist
Signed-off-byline in the message. (more info)