Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/msal #245

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Feature/msal #245

wants to merge 14 commits into from

Conversation

BerendWouters
Copy link
Member

Switch to MS Azure AD B2C

This is a very large PR.
Focus points

  • Removal of IdentityServer in favor of Microsoft Azure AD B2C

Additionals:

  • (Temporary) Disabled seeding of sql data
  • Configuration can be requested at author
  • Still some old userService related functionality exists in the backend, however it should no longer be used by the frontend

@Joren-Thijs
Copy link
Collaborator

Loos good as a first step. Would like to see the commented out code just be removed. This is what git is for.
LGTM!

@@ -158,7 +156,7 @@ export class AdminOverviewComponent implements OnInit, AfterViewInit {
});
dialogRef.afterClosed().subscribe((isConfirmed: boolean) => {
if (isConfirmed) {
this.authService.requestPasswordReset(row.userId);
// this.authService.requestPasswordReset(row.userId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed

@@ -180,7 +178,7 @@ export class LegalguardianOverviewComponent implements OnInit, AfterViewInit {
});
dialogRef.afterClosed().subscribe((isConfirmed: boolean) => {
if (isConfirmed) {
this.authService.requestPasswordReset(row.userId);
// this.authService.requestPasswordReset(row.userId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed?

Comment on lines 79 to 86
// this.authService.isAdmin$.subscribe((res) => {
// this.isAdmin = res;
// this.updateRequirements();
// });
// this.authService.isAuthenticated$.subscribe((res) => {
// this.isAuthenticated = res;
// this.updateRequirements();
// });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed?

@@ -15,7 +15,7 @@ const routes: Routes = [
{
path: 'admin',
component: MainComponent,
canActivate: [AuthGuard],
canActivate: [],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a todo to make sure we add a substitute here?

b2cPolicyNames: IB2CPolicyNames;
}

export const endpoint = 'https://localhost:5001/';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ain't gonna work :|

@@ -30,15 +35,15 @@
</mat-select>
</mat-form-field>
</div>
<div *ngIf="authService.isAdmin$ | async" class="careuser-filter">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the authService.isAdmin$ condition should be left in place here

Comment on lines 38 to 43
// TODO Reintroduce
//await Seed.SeedRolesAsync(serviceScope);
//await Seed.SeedUsersAsync(serviceScope, applicationDbContext, initialAdminPassword);
//await Seed.CheckRolesAsync(serviceScope, applicationDbContext);
//Seed.CreateAPIAndClient(configrationDbContext);
//Seed.SeedIdentityResources(configrationDbContext);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed. Our application shouldn't have to worry about seeding roles or users anymore. We just need to put the expected roles on the controllers



app.UseCors(builder => builder.AllowAnyOrigin().AllowAnyHeader().AllowAnyMethod());
//app.UseIdentityServer();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kill it with fire!

@@ -23,8 +20,8 @@ namespace Singer.Services;

public class AdminUserService : UserService<AdminUser, AdminUserDTO, CreateAdminUserDTO, UpdateAdminUserDTO>, IAdminUserService
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole service can be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@@ -27,6 +27,6 @@ public virtual void Setup()
[TearDown]
public virtual void TearDown()
{
TestDataContext.Dispose();
//TestDataContext.Dispose();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants