Feature/secure services #249
Conversation
All urls that fit the regular expression defined by interceptorUrlRegex have a token set in the cookies of the browser set in the Authorization header as a Bearer token for all outgoing requests.
URLs to services and authentication server are omitted.
There was a problem hiding this comment.
🏓 @dopenguin Unfinished review, I'll add more later.
All in all, I do not see how an application is supposed to use us properly. It seems to me that we require both a configuration parameter for the token and an update mechanism to replace it in time that the login-managing application is required to call in time. Or did I just not see the idea here?
| /* | ||
| * Functions are based on https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsCookies.js | ||
| * and https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsOIDC.js. | ||
| */ |
There was a problem hiding this comment.
Why are we using cookies at all? There seems to be no technical reason to do it, but now we have to worry about cookie laws. Has there been any communication with the customer regarding this? The last state I remember is that we get the token by configuration and thus, it should be fetchable from the store.configuration.token or something like that.
There was a problem hiding this comment.
The configuration option was something I had in mind initially but scrapped quickly.
There are two options that can solve using the token:
- Using the token when calling e.g.
createMap - Requesting the token from the Cookie
If we'd get the token from the call to createMap, we'd have to handle the situation when a token expires and request a new one as the function call is already done. If we'd be doing that with the configuration, the situation would be similar.
By having the token in a cookie, which is usually done by applications that use OIDC tokens for authentication anyway, we can simply request it on the go and not worry about refreshing it on our own.
Do you think an adjustment to the documentation is necessary so that it is clear that interceptorUrlRegex should only be used by integrated clients as we currently do not offer a login module?
There was a problem hiding this comment.
The createMap parameter combined with an action provided to update the token for our application when needed should be the way to go here. This way we initially have a token to start working with and delegate its updates to the leading application that may now refresh and keep it however it desires to – forwarding it to the map is a follow-up step.
This also spares the leading application to be forced to produce a cookie token, which it may not have intended to, or which may also already be in use for another token since that's a very generic word – since the client is supposed to be used in multiple spots eventually, even both may hold true in time.
This way we're also keeping data flow to the etablished channels and don't open up a new one.
Do you think an adjustment to the documentation is necessary so that it is clear that interceptorUrlRegex should only be used by integrated clients as we currently do not offer a login module?
I'd like it to be used in combination with a config parameter and an obligation to refresh it via action, which is a mechanism that should be documented, but is debated atm.
There was a problem hiding this comment.
The name of the token can now be configured, as mentioned in #249 (comment)
Whether the authentication shall be achieved by retrieving a cookie or giving the JWT value via createMap and handling the refresh in POLAR (as well) is now something I am discussing with the relevant person that will integrate POLAR later on.
| export function addInterceptor(interceptorUrlRegex: string) { | ||
| const { fetch: originalFetch } = window | ||
|
|
||
| window.fetch = (resource, originalConfig) => { | ||
| let config = originalConfig | ||
|
|
||
| // @ts-expect-error | Has worked like charm so far. If an error occurs if resource is of type RequestInfo, take another look | ||
| if (interceptorUrlRegex && resource?.match(interceptorUrlRegex)) { | ||
| config = { | ||
| ...originalConfig, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| headers: { Authorization: `Bearer ${getCookie('token')}` }, | ||
| } | ||
| } | ||
|
|
||
| return originalFetch(resource, config) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we share the window element with the outlying application, we're now potentially changing its behaviour. This is quite dangerous and we may break stuff, so I don't deem this mergable, especially since we're plain overriding any headers sent to the target.
There was a problem hiding this comment.
Do you have a suggestion on how to add an interceptor here for only polar instead of the whole application? I don't seem to have come across a reasonable solution so far.
There was a problem hiding this comment.
My first idea was to identify the call source, and only act if it was from within POLAR. Since .caller is deprecated, checking the call stack seems to be a non-solution, and we can't just modify all local requests since OL and the masterportalAPI may start them, too.
The only thing left seems to be the URL itself. We may modify all URLs intended for interception on arrival. Since everything flows through the client creation (either by config or from the services.json), all URLs may run through a check on whether they fit the interceptorUrlRegex. If so, we modify them to hold an appropriate flag.
Ideas on how to mark URLs:
${url}+POLAR_INTERCEPTOR+or another string that will absolutely not randomly appear in the finite time that remains to the universe – however, if there's any operations on the end of the URL, this may or may not break anything?- I wonder whether ?, &, or # may also be made use of, like e.g. in
${url}?polar_interceptor_was_here=true– this may survive further URL modifications since it's technically a correct URL, but I wonder if some URLs are required to not have any appendices like that by e.g. the masterportalAPI? ${scheme}${subdomain}POLAR_INTERCEPTOR.${domain}would hide that as an additional subdomain, and I'm pretty sure it will survive all underlying requests there until fetch is finally used.
Then, in the interceptor, the flag is removed before the real URL is used with the Auth header. This way all fetch calls of the outlying application to the URL are unmodified, and all of our own calls are modified.
That or you don't override all original headers and only add Authorization when it's not there already.
There was a problem hiding this comment.
I've adjusted the configuration so that the name of the token is also configurable.
The changes to the headers is also implemented. d94a7f8
Is it fine like this now or do you deem the marking of the URLs necessary as well?
| setCookie('name', account?.name) | ||
| setCookie('email', account?.email) | ||
| setCookie('username', account?.preferred_username) |
There was a problem hiding this comment.
Why save this? We seem to just require the token. Probably illegal, too, even though we don't use it anyway.
There was a problem hiding this comment.
Applications typically save these values, similarly to #249 (comment). We can keep the example simple you like
There was a problem hiding this comment.
Yes, please keep the example minimal. It will be a guideline for implementers on how to achieve things, and they may mimic this and later wonder what to do with the variables.
| 'email', | ||
| 'expiry', | ||
| ]) | ||
| window.location.reload() |
There was a problem hiding this comment.
Is this an F5? Doesn't it completely eradicate all of the user's progress?
There was a problem hiding this comment.
Correct. But if a user would log out in an application that includes POLAR, this would be the case in most situation. Or am I on the wrong track?
There was a problem hiding this comment.
Oh, the method authenticate is also supposed to do the logout. I expected this to be a token refresh since it calls revokeToken that has grant_type=refresh_token hard-wired. Now that I look at it, the fetch also always uses the URL ''? Is this just unfinished? I guess we come back around to this when the other issues are resolved, the code may have changed until then anyway.
There was a problem hiding this comment.
I can take another look regarding the grant_type (as in: is it the correct one).
The URL is the same thing as with the rest: I'll add this to the repository once we have one that can be put into OSS. Until then, use the URL I provided you through a save channel.
There was a problem hiding this comment.
Oh right, I forgot that it's in the git patch.
There was a problem hiding this comment.
The url used ends with /revoke and the grant_type is also correct for this case.
As mentioned in #249 (comment), the token requests should be done by the leading application and not POLAR. |
|
Co-authored-by: Dennis Sen <[email protected]>
Also, the Authorization header is only overwritten if it is not present yet.
Except #249 (comment) everything should be g2g now. |
Alright. Since that's a pretty fundamental thing, I'll wait for the resolution to that. |
Summary
Add possibility to use services that are authenticated by OIDC by passing a Bearer token when sending requests.
Instructions for local reproduction and review
npm run diplan:devCaveat: If the user tries loading the secure layer while not authenticated, the layer is never shown. This should not be a problem in a proper authentication environment and thus I have not addressed this here.
Pull Request Checklist (for Assignee)
[ ] Screenreader functionality has been manually tested with NVDANot neededUI has been tested in the following tools regarding accessibility (only regarding functionality affected in this PR)