Keycloak Auth2/OIDC #4247
Keycloak Auth2/OIDC #4247
Conversation
This reverts commit c58b60d.
There was a problem hiding this comment.
As mentioned in one of the CRs please revert 3b1391e.
This review does not include the worker part as I would like to see the tests passing before I have a look at it. Passing the "build-and-test-dev-image" pipeline is a hard requirement before merging to feature branches.
Besides from that please fix the linter issues. You can use npm run cleanup to do this. After reverting 3b1391e this should work without manual intervention.
| public async sendMessage<T extends WorkerMessageContent>(receiver: string, msg: T): Promise<void> { | ||
| public async sendMessage<C extends WorkerMessageContent, T extends WorkerMessage<C>>(msg: T): Promise<void> { |
There was a problem hiding this comment.
Why did you remove the receiver parameter? This adds redundancy because the parameter is mandatory anyway and we now always need to specify it in the request.
There was a problem hiding this comment.
T is now to message type, not only the content type.
So msg contains the receiver. This improves type safety when using the method...
There was a problem hiding this comment.
I don't see how this improves type safety. Yes, you are not able to send messages to a wrong receiver with this but I think as the message type normally contains the receiver name it is normally quite clear which receiver to send it to.
Even if we want to do something like this I think there are much better ways to achieve something like this by for example extending the shared worker service or providing methods to generate such messages.
There was a problem hiding this comment.
"Yes, you are not able to send messages to a wrong receiver" ... that is how type safety is improved, and I think you strengthen my point when write, that it's "...normally quite clear...". The compiler can enforce clearity.
This solution is not ideal, but an improvement, I think. Let's discuss a better solution.
There was a problem hiding this comment.
IMHO the added redundancy overweights. I would prefer not doing this in this PR.
|
|
||
| public async getBlobUrl(url: string): Promise<string | null> { | ||
| await this.authTokenService.waitForValidToken(); | ||
| try { | ||
| if (this.blobCache.has(url)) { | ||
| return this.blobCache.get(url); | ||
| } | ||
|
|
||
| // Fetch the resource as a blob | ||
| const response = await fetch(url, { headers: { Authentication: `Bearer ${this.authTokenService.rawAccessToken}` } }); | ||
| if (!response.ok) { | ||
| throw new Error('Network response was not ok'); | ||
| } | ||
| const blob = await response.blob(); | ||
|
|
||
| // Create a blob URL and cache it | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| this.blobCache.set(url, blobUrl); | ||
|
|
||
| return blobUrl; | ||
| } catch (error) { | ||
| console.error('Error loading image:', error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is unnecessary. We are using cache control headers for mediafiles. See #1915 for further details.
There was a problem hiding this comment.
The main perpose is getting the blob as an object URL. The fetch call might return a cached file but still response.blob() will create a new in-memory representation of the file.
There was a problem hiding this comment.
As I said.
TLDR; We had this. With cache control headers you can achieve something like this with native browser apis. This behavior is disabled in dev setup.
There was a problem hiding this comment.
There was a problem hiding this comment.
Keep it simple. Just use this.authTokenService.rawAccessToken instead of the observable. There is no benefit in keeping track of token changes here.
client/src/app/openslides-main-module/interceptors/auth-token.interceptor.ts
Outdated
Show resolved
Hide resolved
| public constructor(private authTokenService: AuthTokenService) { | ||
| this.authTokenService.accessTokenObservable.subscribe(token => { | ||
| if (token) { | ||
| console.log(`Access token: ${token.rawAccessToken}`); |
There was a problem hiding this comment.
| console.log(`Access token: ${token.rawAccessToken}`); |
| @@ -63,9 +63,6 @@ export class OpenSlidesRouterService { | |||
| private updateService: UpdateService, | |||
| private operator: OperatorService | |||
| ) { | |||
| _auth.logoutObservable.subscribe(() => { | |||
There was a problem hiding this comment.
Also remove from constructor when unused.
| @@ -25,19 +25,15 @@ export class OpenSlidesService { | |||
| * {@method afterLoginBootup}. If not, redirect the user to the login page. | |||
| */ | |||
| public async bootup(): Promise<void> { | |||
| const online = await this.authService.doWhoAmIRequest(); | |||
| const online = await this.authService.checkOnline(); | |||
There was a problem hiding this comment.
If I understand correctly this now always does a request to a health endpoint on bootup. I think that would be unnecessary.
| @@ -7,7 +7,7 @@ import { SiteWrapperComponent } from './modules/site-wrapper/components/site-wra | |||
|
|
|||
| const routes: Routes = [ | |||
| { | |||
| path: `login`, | |||
| path: `idp`, | |||
There was a problem hiding this comment.
We might want to add a redirect if we really want to change this.
There was a problem hiding this comment.
Move to a definitions folder and rename to auth.ts. Same for the other interfaces in this folder.
…interceptor.ts Co-authored-by: Bastian Rihm <[email protected]>
Co-authored-by: Bastian Rihm <[email protected]>
No description provided.