fix: oauth2 inheritance [INS-5023]

[ryan-willis]

fixes #7508
fixes #7880
fixes #8374

This change alters the authentication model for OAuth2 requests to add an additional optional field that is derived at request time and used to inherit the correct token when executing a request from within a folder that has OAuth2 configured.

EDIT: This change alters the RenderedRequest type to include an _authParentId field that is used to determine what parentId to use when storing/retrieving an OAuth2 token.

The smoke test has been updated with the reproduction steps from the linked issues to confirm that it uses the "nearest" folder's token every attempt, so instead of storing a token with a relationship to the request itself, it maintains the relationship to the folder.

[marckong]

Simple and to the point PR!

[jackkav]

what are the consequences of creating a relationship like this?

[marckong]

hm good question. I was looking through the code and we are just doing getOrCreateByParentId. It just reads an entity. Only if there is none, then it creates, which I think happens regardless of this change. I can't think of any concerning consequence from this change.

[ryan-willis]

The only thing that comes to mind is how differently we handle folder deletes vs request deletes, and the delete folder code might not look for token entities, leaving them dangling. I'll take a look to see

EDIT: looks like we're clear on that front, any time anything is removed we gather descendants and remove those too

[jackkav]

hmm, I think. we have a database normalisation problem there.
Our database uses parentIds explicitly for joining different objects. There should never be an object with two parentIds. Because the withDescendents code could not compute children if it needs to search every object within the db objects.

This authentication object is a property of request. therefore.

request:{
  parentId:'afolder?',
  authentication:{
    parentId:'somethingelse?'
  }
}

it now becomes possible for a request to have a parentId and a authentication object to have a different parentId.

I would suggest two direction this PR could take:

1. split and migrate request.authentication into a new db object with a parentIds or request or folder.
2. improve the code which requires this parentId, to avoid it.

[ryan-willis]

Ah, I see. The authentication object should only have that field on it during a request, and not make it to the db, but if it's doing that, I'm leaning toward option 2 and wrapping that up into a

{
  requestOrGroupId: "parentId",
  authentication: original
}

for the OAuth paths, what do you think?

[marckong]

@jackkav's concerns are valid. I think 1 might be a better option but would require migration path for previous versions. Option 2 would be a good alternative without the db migration path. I didn't quite understand the 2 option @ryan-willis you are proposing. Could you help me understand?

[ryan-willis]

I'm thinking of changing interface for these methods, rather than the models, let me draft a change that targets this branch that will explain it better

[ryan-willis]

I've confirmed that this field doesn't persist on the request's authentication object, and is only utilized in the active context of a request action.

I can rename this field to make it less conflating, something like sourceId or requestOrGroupId (since we use that for the same purpose) if that's preferred

[jackkav]

so the auth object is updated for the purpose of something that happens later on, but not persisted in the database, in which case the db object shouldnt be modified at all, perhaps create a new code only object. Let's discuss on zoom since I build the first version I might have some insights that are hard to recall in a PR review.

[ryan-willis]

@jackkav that's what I was leaning toward with option 2, I'll update this PR with that

[jackkav]

Some things come to mind here

• This e2e test and implemenation decision here would be much clearer if we add a UX to view oauth2 tokens, even just a tooltip
• the naming closestAuthId would be clearer than authParentId since its not a direct parent in the same sense its used in the db model, but a indirect relationship does exist.
• An alternative implementation could be re-running the closest auth algorithm to get the id in getExistingAccessTokenAndRefreshIfExpired every time instead of storing the closest auth id and carrying it around.

This one hurts the brain a bit

[jackkav]

would prefer a stateless approach but happy to move it forward now the naming is clarified, would be great to plan some improvements to this feature too