Skip to content

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!

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

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?

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 :|

</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!


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

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.

4 participants