feat: add `withAppInitializerAuthCheck` as a feature for `provideAuth` #2006

kyubisation · 2024-09-18T13:46:01Z

This PR adds a feature withAppInitializerAuthCheck to handle OAuth callbacks during app initialization phase. This replaces the need to manually call OidcSecurityService.checkAuth(...) or OidcSecurityService.checkAuthMultiple(...).

Closes #1926

FabianGosebrink · 2024-09-20T06:38:13Z

Thanks, that is a nice idea. Love it. This would also mean that we have to update the samples, is this correct?

kyubisation · 2024-09-20T08:24:07Z

Thank you 😃
As it is an opt-in, I was not sure how deep to integrate it into existing code. Is there a specific example or range of examples where you would like to see this used?

FabianGosebrink · 2024-09-20T09:11:17Z

We have to thank YOU for this PR ❤️

It would be nice to see the either/or comparison in the docs, because the users have to erase the checkAuth(...) step, which gets forgotten really easily. However, maybe we can enhance an existing sample "sample-code-flow-azuread" or "sample-code-flow-auth0", comment out the added code and comment it like you did on the docs, so that the users can see what they would have to change. What do you think?

kyubisation · 2024-09-20T09:35:15Z

I think that's fine. 👍
I'm also fine with creating a new example, if that is preferred.
I'll have a look to adapt this, but it might be next week.

Maybe one question is; Should this potentially become the recommendation (in examples) in the future? While we at SBB have been using this approach for a while, I am hesitant to suggest a change in the general approach, as I am not a maintainer of this project. We can also wait a while before evaluating whether a change would be an improvement.

FabianGosebrink · 2024-09-20T09:43:32Z

Generally: Yes, I would recommend this. In practice and out of experience, I know that auth and so also this lib is used in ways you can not even imagine. So if it fits for EVERY case? I doubt it, because auth is very special. However, we want to provide a go to and fast solution to do your auth, so your feature is self explaining and with the docs we can also advise for the alternative solution. If we see that your feature establishes as stable, when can absolutely take it as the recommended solution. I, personally, will absolutely use it that way :)

Next week is absolutely fine, as you like, and I would not create an extra sample for this.

Are you using it heavy at the SBB?

kyubisation · 2024-09-20T10:03:17Z

That sounds reasonable. 👍
And yes, I am unfortunately very much aware of the many ways auth can be used (and misused).

No, not at the moment, but that statement might be misleading.
We previously used the keycloak library or https://www.npmjs.com/package/angular-oauth2-oidc. This year we evaluated this library (and others), with the result that this library matches our use case the best.
Due to this, it is now the recommended authentication library for new projects. but that does not affect our hundreds of existing Angular applications that might or might not migrate the auth library.
I hope that puts it into perspective.

FabianGosebrink · 2024-09-20T10:45:59Z

Hey, thanks. Yeah, that is what I also see at customer projects. New project use the new guidelines, and old ones might or might not be migrated. Nice. Let me know if you are missing any feature, have a question or need something. We leave this PR for now and

enhance one sample with the new way commented out
add docs to have the "old" and "new" way side by side or underneath, so that we can link to it

Your PR is awesome. Thanks so much.

kyubisation · 2024-09-27T15:07:26Z

@FabianGosebrink I just realized that currently the only example using provideAuth is the sample-code-flow-standalone. Should I enhance that example or should e.g. the azuread example be migrated to standalone?

timdeschryver

I like this feature!

I think updating the standalone example is fine for now, as we can use it to verify that this feature works as expected.

With standalone becoming the default I think we should update all examples in seperate PR(s).
We can leave one using NgModule to also cover.
I suppose the docs will need some enhancements too.

docs/site/angular-auth-oidc-client/docs/documentation/configuration.md

kyubisation · 2024-09-30T06:20:50Z

I have migrated the azuread example, as this leaves an example with the current/previous standalone configuration. I have also extended the documentation a little bit, as discussed.
Feel free to have another look and tell me, if I still need to address something.

timdeschryver

LGTM 👍
The comment is a personal one, but isn't "blocking".

timdeschryver · 2024-10-01T17:47:07Z

docs/site/angular-auth-oidc-client/docs/documentation/configuration.md

+If you prefer to manually check OAuth callback state, you can omit
+`withAppInitializerAuthCheck`. However, you then need to call
+`OidcSecurityService.checkAuth(...)` or
+`OidcSecurityService.checkAuthMultiple(...)` manually in your
+`app.component.ts` (or a similar code path that is called early in your app).
+
+```ts
+// Shortened for brevity
+...
+export class AppComponent implements OnInit {
+  private readonly oidcSecurityService = inject(OidcSecurityService);
+
+  ngOnInit(): void {
+    this.oidcSecurityService
+      .checkAuth()
+      .subscribe(({ isAuthenticated, accessToken }) => {
+        console.log('app authenticated', isAuthenticated);
+        console.log(`Current access token is '${accessToken}'`);
+      });
+  }
+...
+```


We can also refer to the checkAuth API, this keeps the documentation in one place.

Could you explain this in more detail? I'm not sure I fully understand what you mean.

The docs already provide information and an example for checkAuth and checkAuthMultiple.
This means we can (in my opinion) just refer to the documentation, instead of including it here.
This prevents duplicates and makes it easier to change/update the docs later.

We can remove this section and update the text above the example, something as:

Additionally, you can use the feature function `withAppInitializerAuthCheck` to handle OAuth callbacks during app initialization phase. This replaces the need to manually call [`OidcSecurityService.checkAuth(...)`](/docs/documentation/public-api#checkauthurl-string-configid-string) or [`OidcSecurityService.checkAuthMultiple(...)`](/docs/documentation/public-api#checkauthmultipleurl-string) within the `app.component.ts` file (or a similar code path that is called early in your app).

As a result we don't repeat ourselves.

I can do that, but @FabianGosebrink requested to add docs to have the "old" and "new" way side by side or underneath, so that we can link to it.
I'm fine with either. Should I adapt, as suggested by @timdeschryver?

Oh nevermind my comment then :)

Easy, I just wanted it to make it as easy as possible for the users to use. I am fine with both :)

FabianGosebrink · 2024-10-04T13:12:29Z

Thank you both for being so active here :)

FabianGosebrink · 2024-10-08T13:11:51Z

@timdeschryver are you okay with merging this?
@kyubisation You finished with implementing this?

kyubisation · 2024-10-08T14:58:50Z

Yes, I was merely waiting for any advice on how to proceed with the raised issue.
If it is fine, as it is, it can be merged. If I should adapt the documentation, I can do that if desired.

timdeschryver

Looks good to me

FabianGosebrink · 2024-10-09T17:07:23Z

Thanks :)

kyubisation · 2024-10-09T18:24:17Z

Thank you both for the review, feedback and merge. 😃 👍

FabianGosebrink · 2024-10-09T19:18:11Z

Thank YOU for the work you did!! This lib grows because of contributions from people like you. You did amazing. Thanks a million!

konstantinvlasenko · 2024-10-10T18:09:36Z

Question. I am using 18.0.1
I don't see withAppInitializerAuthCheck
Am I missing something?

Trying to run this example https://github.com/damienbod/angular-auth-oidc-client/tree/main/projects/sample-code-flow-azuread
But getting error TS2305: Module '"angular-auth-oidc-client"' has no exported member 'withAppInitializerAuthCheck'

kyubisation · 2024-10-11T15:25:58Z

This feature has not been released yet. I am not overly familiar with the release cycles of this library, but I assume this will be released with version 19 in November.

FabianGosebrink · 2024-10-11T15:46:25Z

Yes, we will run the last tests and then release with this feature included. Thanks!

damienbod · 2024-10-11T17:06:16Z

I will do some testing, docs and try to release this weekend

Greetings and thanks for the PR, Damien

goncard · 2024-10-11T23:03:42Z

Question: when we are guarding all the routes with AutoLoginPartialRoutesGuard, will this feature replace the need to call checkAuth on app component or callback component? I am calling it on the callback component which is my public route but if it will not be needed anymore, the callback component will basically be useless. Does it make sense to keep a empty component? What would you do in this case?

Thanks.

timdeschryver reviewed Sep 27, 2024

View reviewed changes

docs/site/angular-auth-oidc-client/docs/documentation/configuration.md Outdated Show resolved Hide resolved

kyubisation added 3 commits September 30, 2024 08:07

feat: add withAppInitializerAuthCheck as a feature for provideAuth

5f76734

refactor: migrate azuread example to standalone and use new feature

496425b

docs: extend documentation for standalone config

49a71ee

kyubisation force-pushed the feat-app-initializer-auth-check branch from bce9443 to 49a71ee Compare September 30, 2024 06:19

timdeschryver approved these changes Oct 1, 2024

View reviewed changes

timdeschryver approved these changes Oct 9, 2024

View reviewed changes

FabianGosebrink merged commit 14f032c into damienbod:main Oct 9, 2024
9 checks passed

kyubisation deleted the feat-app-initializer-auth-check branch October 9, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `withAppInitializerAuthCheck` as a feature for `provideAuth` #2006

feat: add `withAppInitializerAuthCheck` as a feature for `provideAuth` #2006

kyubisation commented Sep 18, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024 •

edited

Loading

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 27, 2024

timdeschryver left a comment

kyubisation commented Sep 30, 2024

timdeschryver left a comment

timdeschryver Oct 1, 2024

kyubisation Oct 2, 2024

timdeschryver Oct 2, 2024 •

edited

Loading

kyubisation Oct 3, 2024

timdeschryver Oct 3, 2024 •

edited

Loading

FabianGosebrink Oct 4, 2024

FabianGosebrink commented Oct 4, 2024

FabianGosebrink commented Oct 8, 2024

kyubisation commented Oct 8, 2024

timdeschryver left a comment

FabianGosebrink commented Oct 9, 2024

kyubisation commented Oct 9, 2024

FabianGosebrink commented Oct 9, 2024

konstantinvlasenko commented Oct 10, 2024 •

edited

Loading

kyubisation commented Oct 11, 2024

FabianGosebrink commented Oct 11, 2024

damienbod commented Oct 11, 2024

goncard commented Oct 11, 2024 •

edited

Loading

feat: add withAppInitializerAuthCheck as a feature for provideAuth #2006

feat: add withAppInitializerAuthCheck as a feature for provideAuth #2006

Conversation

kyubisation commented Sep 18, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024 • edited Loading

kyubisation commented Sep 20, 2024

FabianGosebrink commented Sep 20, 2024

kyubisation commented Sep 27, 2024

timdeschryver left a comment

Choose a reason for hiding this comment

kyubisation commented Sep 30, 2024

timdeschryver left a comment

Choose a reason for hiding this comment

timdeschryver Oct 1, 2024

Choose a reason for hiding this comment

kyubisation Oct 2, 2024

Choose a reason for hiding this comment

timdeschryver Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

kyubisation Oct 3, 2024

Choose a reason for hiding this comment

timdeschryver Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

FabianGosebrink Oct 4, 2024

Choose a reason for hiding this comment

FabianGosebrink commented Oct 4, 2024

FabianGosebrink commented Oct 8, 2024

kyubisation commented Oct 8, 2024

timdeschryver left a comment

Choose a reason for hiding this comment

FabianGosebrink commented Oct 9, 2024

kyubisation commented Oct 9, 2024

FabianGosebrink commented Oct 9, 2024

konstantinvlasenko commented Oct 10, 2024 • edited Loading

kyubisation commented Oct 11, 2024

FabianGosebrink commented Oct 11, 2024

damienbod commented Oct 11, 2024

goncard commented Oct 11, 2024 • edited Loading

feat: add `withAppInitializerAuthCheck` as a feature for `provideAuth` #2006

feat: add `withAppInitializerAuthCheck` as a feature for `provideAuth` #2006

FabianGosebrink commented Sep 20, 2024 •

edited

Loading

timdeschryver Oct 2, 2024 •

edited

Loading

timdeschryver Oct 3, 2024 •

edited

Loading

konstantinvlasenko commented Oct 10, 2024 •

edited

Loading

goncard commented Oct 11, 2024 •

edited

Loading