Closed vikramkh closed 5 years ago
Sorry for the overload of questions and thank you so much for responding to the previous ones.
Hi! I just wondering how come there is no way to update the user_tenants table when a new user is added? Shouldn't it be updated with the addition of a user? Should we manually add a controller to do this or am I missing something? Because how would the program know what permissions the user has.
Hello @vikramkh , we intentionally left this part from the boilerplate, as this business logic may vary from application to application. Like, for example, in some cases, whenever a user is created (say, sign up), a new tenant will be created for the customer name mentioned in request. In other cases, a new user will be created by another user, so in that case, the new user will always belong to same tenant as the creator. As you can see, there are various different logic that can happen here. So, it cannot be made a part of boilerplate. But I can surely give you some direction on how it can be done. There are multiple ways, again depending upon your requirement.
// This is added to constructor for injecting current user getter
// @inject.getter(AuthenticationBindings.CURRENT_USER)
// private readonly getUser: Getter<User | UserDTO | undefined>,
@authenticate(STRATEGY.BEARER)
@authorise([PermissionKey.CreateAnyUser, PermissionKey.CreateTenantUser])
@post( '/users', {
responses: {
[STATUS_CODE.OK]: {
description: 'User data model instance',
content: {
[CONTENT_TYPE.JSON]: {schema: {'x-ts-type': UserDTO}},
},
},
},
})
async create(@requestBody() user: UserDTO): Promise<UserDTO> {
try {
// First look for permission errors in request
await this._checkCreateErrors(user);
const userCreated: UserDTO = await super.create(new UserDTO(user));
const userTenant: UserTenant = new UserTenant();
userTenant.role_id = user.role.id;
userTenant.status = user.status || 'active';
userTenant.tenant_id = user.defaultTenant.id;
userTenant.user_id = userCreated.id;
await this.userTenantRepository.create(
userTenant,
);
return userCreated;
} catch (error) {
if (error.status && error.statusCode) {
throw error;
} else if (error) {
this.logger.error(`Error occured while creating user.
Error => ${error.toString()}
Detail => ${error.detail}
Message => ${error.message}
`);
}
throw new HttpErrors.UnprocessableEntity(
UserTenantErrorKeys.UserCreateFailed,
);
}
}
private _checkCreatePermissionsForTenant(
currentUser: UserDTO,
user: UserDTO,
) {
return (
currentUser.permissions &&
(PermissionsManager.isAllowed(currentUser.permissions, [
PermissionKey.CreateAnyUser,
]) ||
(PermissionsManager.isAllowed(currentUser.permissions, [
PermissionKey.CreateTenantUser,
]) &&
currentUser.tenant &&
currentUser.tenant.id === user.defaultTenant.id))
);
}
private async _checkCreateErrors(user: UserDTO) {
const currentUser = await this.getUser();
if (!currentUser || currentUser instanceof User) {
throw new HttpErrors.Forbidden(AuthoriseErrorKeys.NotAllowedAccess);
} else if (
currentUser instanceof UserDTO &&
!this._checkCreatePermissionsForTenant(currentUser, user)
) {
throw new HttpErrors.Forbidden(AuthoriseErrorKeys.NotAllowedAccess);
}
}
Please note I have skipped some of the implementation part above as that is beyond the scope of this. It's shared just to give you an idea on how to do this. Feel free to implement according to your application logic. If it's still not clear enough, I can try give you full logic but for that I would need what application use case you have. Hope this helps.
Sorry for the overload of questions and thank you so much for responding to the previous ones.
That's no problem @vikramkh . We are happy to help. If you are finding this project useful, we'll apprecate if you can star it. :P
Thanks so much for the help! I understand. Say I were to post a user to the users table and then manually update the user_tenant table to reflect role number 3. Would something be wrong with this? I did this, and the user that is associated with role 3 (but still tenant 1) in the user_tenant table seems to be able to have authorization to everything still (all permissions that role 1 has). Is that expected behavior based on what I did? I wanted to restrict the permissions to role 3
No that's not expected behaviour. If you assign role 3 to a user via user_tenant, it should have access to permissions specified for role 3 only.
Can you check what permissions you have added to role 3 in roles table ? Check data in column permissions. Its an array of permission strings. If it is same as role 1, then you need to fix that.
I checked and made sure in the user_tenants, the only thing that is the same is the tenant, but the role id is completely different and the permissions are different. Something seems to be happening in these three lines:
I even changed the tenant_id in the user_tenants table and yet it is giving me the wrong tenant as well as the wrong role, but the right AuthUser. THis is my users table:
This is my user_tenants table:
And this is what is being return in this console log:
Any thoughts as to why it is returning the wrong tenant/role even though the table specifies the right ones? If I console.log(userTenant) I get the correct row of the table.
On a separate note, is type supposed to be in the tenant model? It is in the table schema but not the model
I think there is some understanding gap here. AuthUser is showing you correct data. See carefully its returning defaulttenant as 2. Also there is too much info you are logging which is making it difficult to deduce. To make it simpler, just do console log after the below code.
// Find the user-tenant table entry for the currently logging in user id and its default tenant id value.
const userTenant = await this.userTenantRepo.findOne({
where: {
userId: user.getId(),
tenantId: user.defaultTenant,
},
});
// Making sure this tenant exists in db and this user belongs to it.
if (!userTenant) {
throw new HttpErrors.Unauthorized(
AuthenticateErrorKeys.UserDoesNotExist,
);
} else if (userTenant.status !== 'active') {
throw new HttpErrors.Unauthorized(AuthenticateErrorKeys.UserInactive);
}
// Create user DTO for payload to JWT
const authUser: AuthUser = new AuthUser(user);
// Fetch tenant details for the user
authUser.tenant = await this.userTenantRepo.tenant(userTenant.id);
// Fetch role details for this user in this tenant.
const role = await this.userTenantRepo.role(userTenant.id);
// Fetch any additional permissions override at user-level
const utPerms = await this.utPermsRepo.find({
where: {
userTenantId: userTenant.id,
},
fields: {
permission: true,
allowed: true,
},
});
// Update the permissions into AuhUser DTO object for returning in response
authUser.permissions = this.getUserPermissions(utPerms, role.permissions);
// Update the role into AuhUser DTO object for returning in response
authUser.role = role.roleKey.toString();
console.log(authUser);
Also, you can keep the same tenant. That's no issue.
On a separate note, is type supposed to be in the tenant model? It is in the table schema but not the model
That's a good catch. It need not be there at all. Need to remove it from db schema file. Can you raise a separate issue for this ?
Just re-verified. We need the type in tenant table. So it requires to be added to tenant.model. Just add below to the tenant.model.ts.
@property({
type: 'string',
required: true,
})
type: string;
Purpose is here.
I think there is some understanding gap here. AuthUser is showing you correct data. See carefully its returning defaulttenant as 2. Also there is too much info you are logging which is making it difficult to deduce. To make it simpler, just do console log after the below code.
// Find the user-tenant table entry for the currently logging in user id and its default tenant id value. const userTenant = await this.userTenantRepo.findOne({ where: { userId: user.getId(), tenantId: user.defaultTenant, }, }); // Making sure this tenant exists in db and this user belongs to it. if (!userTenant) { throw new HttpErrors.Unauthorized( AuthenticateErrorKeys.UserDoesNotExist, ); } else if (userTenant.status !== 'active') { throw new HttpErrors.Unauthorized(AuthenticateErrorKeys.UserInactive); } // Create user DTO for payload to JWT const authUser: AuthUser = new AuthUser(user); // Fetch tenant details for the user authUser.tenant = await this.userTenantRepo.tenant(userTenant.id); // Fetch role details for this user in this tenant. const role = await this.userTenantRepo.role(userTenant.id); // Fetch any additional permissions override at user-level const utPerms = await this.utPermsRepo.find({ where: { userTenantId: userTenant.id, }, fields: { permission: true, allowed: true, }, }); // Update the permissions into AuhUser DTO object for returning in response authUser.permissions = this.getUserPermissions(utPerms, role.permissions); // Update the role into AuhUser DTO object for returning in response authUser.role = role.roleKey.toString(); console.log(authUser);
Also, you can keep the same tenant. That's no issue.
Sorry for all the screen shots. So basically, auth user is correct, but the return value from:
await this.userTenantRepo.tenant(userTenant.id);
and await this.userTenantRepo.roles(userTenant.id);
are incorrect, and are returning the wrong tenant and role, which is what is causing the problem. Even though the userTenant.id is correct in referencing the table row, the repo returns the wrong value. It seems to be a very strange error.
Can you please share your full createJWT function code here ?
`private async createJWT(
payload: ClientAuthCode<User>,
authClient: AuthClient,
): Promise<TokenResponse> {
try {
let user: User | undefined;
if (payload.user) {
user = payload.user;
} else if (payload.userId) {
user = await this.userRepo.findById(payload.userId);
}
if (!user) {
throw new HttpErrors.Unauthorized(
AuthenticateErrorKeys.UserDoesNotExist,
);
}
const userTenant = await this.userTenantRepo.findOne({
where: {
userId: user.getId(),
tenantId: user.defaultTenant,
},
});
if (!userTenant) {
throw new HttpErrors.Unauthorized(
AuthenticateErrorKeys.UserDoesNotExist,
);
} else if (userTenant.status !== 'active') {
throw new HttpErrors.Unauthorized(AuthenticateErrorKeys.UserInactive);
}
// Create user DTO for payload to JWT
const authUser: AuthUser = new AuthUser(user);
authUser.tenant = await this.userTenantRepo.tenant(userTenant.id);
const role = await this.userTenantRepo.role(userTenant.id);
console.log(role)
const utPerms = await this.utPermsRepo.find({
where: {
userTenantId: userTenant.id,
},
fields: {
permission: true,
allowed: true,
},
});
authUser.permissions = this.getUserPermissions(utPerms, role.permissions);
authUser.role = role.roleKey.toString();
const accessToken = jwt.sign(
authUser.toJSON(),
process.env.JWT_SECRET as string,
{
expiresIn: authClient.accessTokenExpiration,
issuer: process.env.JWT_ISSUER,
},
);
const size = 32,
ms = 1000;
const refreshToken: string = crypto.randomBytes(size).toString('hex');
// Set refresh token into redis for later verification
await this.refreshTokenRepo.set(
refreshToken,
{clientId: authClient.clientId, userId: user.id},
{ttl: authClient.refreshTokenExpiration * ms},
);
return new TokenResponse({accessToken, refreshToken});
} catch (error) {
if (HttpErrors.HttpError.prototype.isPrototypeOf(error)) {
throw error;
} else {
throw new HttpErrors.Unauthorized(AuthErrorKeys.InvalidCredentials);
}
}
}`
Can you check which block is hit here ?
if (payload.user) {
user = payload.user;
} else if (payload.userId) {
user = await this.userRepo.findById(payload.userId);
}
and what is the value of user after this.
the else if is being hit.
The value of user is still the correct value that I logged in with, username: string
Can you console.log(user) after the above if-else block and share the result ? Meanwhile, I am trying to show you a sample with another user logging in with different permissions.
Sure thing! This is the return from console.log(user) after the above if-else block:
Can you log this and share result ?
const userTenant = await this.userTenantRepo.findOne({
where: {
userId: user.getId(),
tenantId: user.defaultTenant,
},
});
console.log(userTenant);
console.log(user.getId());
console.log(user.defaultTenant);
See the values are absolutely correct until now. So far so good. Now lets log below.
// Create user DTO for payload to JWT
const authUser: AuthUser = new AuthUser(user);
authUser.tenant = await this.userTenantRepo.tenant(userTenant.id);
console.log(authUser.tenant)
console.log(userTenant.id)
It seems here is where the values become wrong. The tenant returned is not the right tenant
Can you share the result of select * from tenants ? Also share the code of user-tenant.repository.ts.
tenants:
import {Getter, inject} from '@loopback/core';
import {BelongsToAccessor, repository} from '@loopback/repository';
import {PgdbDataSource} from '../datasources';
import {Role, Tenant, User, UserTenant} from '../models';
import {DefaultSoftCrudRepository} from './default-soft-crud.repository.base';
import {RoleRepository} from './role.repository';
import {TenantRepository} from './tenant.repository';
import {UserRepository} from './user.repository';
export class UserTenantRepository extends DefaultSoftCrudRepository<
UserTenant,
typeof UserTenant.prototype.id
> {
public readonly tenant: BelongsToAccessor<
Tenant,
typeof UserTenant.prototype.id
>;
public readonly user: BelongsToAccessor<User, typeof UserTenant.prototype.id>;
public readonly role: BelongsToAccessor<Role, typeof UserTenant.prototype.id>;
constructor(
@inject('datasources.pgdb') dataSource: PgdbDataSource,
@repository.getter(TenantRepository)
tenantRepositoryGetter: Getter<TenantRepository>,
@repository.getter(UserRepository)
userRepositoryGetter: Getter<UserRepository>,
@repository.getter(RoleRepository)
roleRepositoryGetter: Getter<RoleRepository>,
) {
super(UserTenant, dataSource);
this.tenant = this._createBelongsToAccessorFor(
'tenant_id',
tenantRepositoryGetter,
);
this.user = this._createBelongsToAccessorFor(
'user_id',
userRepositoryGetter,
);
this.role = this._createBelongsToAccessorFor(
'role_id',
roleRepositoryGetter,
);
}
}
Hmm, thats really strange. Lets do some changes and try now. Replace the code with below.
// Create user DTO for payload to JWT
const authUser: AuthUser = new AuthUser(user);
// authUser.tenant = await this.userTenantRepo.tenant(userTenant.id);
authUser.tenant = await this.tenantRepo.findById(userTenant.tenantId);
console.log(authUser.tenant)
console.log(userTenant.tenantId)
Remember to inject the tenant repo into constructor
@repository(TenantRepository)
public tenantRepo: TenantRepository,
Rebuild and run again.
Amazing!
Should I do the same for roles?
That's weird and really strange. Anyways, for now, yes do the same for role as well. I'll try to debug the original problem in a separate branch later on and let you know what was going on. For now, your problem is resolved this way I hope.
Yea I agree that is very strange. Thank you so much for the help! I'll keep a lookout for it! Yes this solves it thank you!
Hi! I just wondering how come there is no way to update the user_tenants table when a new user is added? Shouldn't it be updated with the addition of a user? Should we manually add a controller to do this or am I missing something? Because how would the program know what permissions the user has.
Hello @vikramkh , we intentionally left this part from the boilerplate, as this business logic may vary from application to application. Like, for example, in some cases, whenever a user is created (say, sign up), a new tenant will be created for the customer name mentioned in request. In other cases, a new user will be created by another user, so in that case, the new user will always belong to same tenant as the creator. As you can see, there are various different logic that can happen here. So, it cannot be made a part of boilerplate. But I can surely give you some direction on how it can be done. There are multiple ways, again depending upon your requirement.
- Create separate controllers for tenant and user-tenant. Hit multiple APIs from your front end to accomplish your logic. Drawback here is business logic of user creation moves to front end, which is a poor design.
- Add the business logic to user create method in controller. In this case, you will have to replace the request body type of create method with a different model which we call a DTO (Data Transfer Object), so that you can accept tenant and other information into the same request. Drawback here is create method becomes complex. Here is a sample implementation.
// This is added to constructor for injecting current user getter // @inject.getter(AuthenticationBindings.CURRENT_USER) // private readonly getUser: Getter<User | UserDTO | undefined>, @authenticate(STRATEGY.BEARER) @authorise([PermissionKey.CreateAnyUser, PermissionKey.CreateTenantUser]) @post( '/users', { responses: { [STATUS_CODE.OK]: { description: 'User data model instance', content: { [CONTENT_TYPE.JSON]: {schema: {'x-ts-type': UserDTO}}, }, }, }, }) async create(@requestBody() user: UserDTO): Promise<UserDTO> { try { // First look for permission errors in request await this._checkCreateErrors(user); const userCreated: UserDTO = await super.create(new UserDTO(user)); const userTenant: UserTenant = new UserTenant(); userTenant.role_id = user.role.id; userTenant.status = user.status || 'active'; userTenant.tenant_id = user.defaultTenant.id; userTenant.user_id = userCreated.id; await this.userTenantRepository.create( userTenant, ); return userCreated; } catch (error) { if (error.status && error.statusCode) { throw error; } else if (error) { this.logger.error(`Error occured while creating user. Error => ${error.toString()} Detail => ${error.detail} Message => ${error.message} `); } throw new HttpErrors.UnprocessableEntity( UserTenantErrorKeys.UserCreateFailed, ); } } private _checkCreatePermissionsForTenant( currentUser: UserDTO, user: UserDTO, ) { return ( currentUser.permissions && (PermissionsManager.isAllowed(currentUser.permissions, [ PermissionKey.CreateAnyUser, ]) || (PermissionsManager.isAllowed(currentUser.permissions, [ PermissionKey.CreateTenantUser, ]) && currentUser.tenant && currentUser.tenant.id === user.defaultTenant.id)) ); } private async _checkCreateErrors(user: UserDTO) { const currentUser = await this.getUser(); if (!currentUser || currentUser instanceof User) { throw new HttpErrors.Forbidden(AuthoriseErrorKeys.NotAllowedAccess); } else if ( currentUser instanceof UserDTO && !this._checkCreatePermissionsForTenant(currentUser, user) ) { throw new HttpErrors.Forbidden(AuthoriseErrorKeys.NotAllowedAccess); } }
- You can also make use of hasmany relation to perform this. But you will still need to write similar code as above in your controller method.
Please note I have skipped some of the implementation part above as that is beyond the scope of this. It's shared just to give you an idea on how to do this. Feel free to implement according to your application logic. If it's still not clear enough, I can try give you full logic but for that I would need what application use case you have. Hope this helps.
For this, couldn't you instead just create an Object that contains both the information for a user and a tenant, have that be passed into the create method:
@requestBody() user: UserTenantInfo
And then create two repositories, one for the User and one for the Tenant by parsing the information in that object? Then you could call create from each repository and pass in the respective objects? That way you would not have to use a Data Transfer Object, which I have been reading up on and it says it is an expensive operation.
For this, couldn't you instead just create an Object that contains both the information for a user and a tenant, have that be passed into the create method:
What you are suggesting is actually a DTO only. That is what I suggested exactly.
And then create two repositories, one for the User and one for the Tenant by parsing the information in that object? Then you could call create from each repository and pass in the respective objects?
Have a look at my code. I am doing a similar stuff that you suggested. You are naming it UserTenantInfo. Thats the only major difference. Another difference is you want to create it by combination of persistent models. You can do that. But if you create a DTO by combining such models, then, you are again tight coupling it with persistent models. That defeats the purpose of having such DTOs. It's an anti-pattern. Just think about User model contains password. How will you exclude password from request/response object in such case ? There can be many such properties in DB models which you want to exclude from API signature. Think about created_by, modified_by, deleted flag. You may not need these to be there in API.
Read a pretty good explanation here.
DTO stands for Data Transfer Object.
This pattern was created with a very well defined purpose: transfer data to remote interfaces, just like web services. This pattern fits very well in a REST API and DTOs will give you more flexibility in the long run.
REST resources representations don't need to have the same attributes as the persistence models: you may need to omit, add or rename attributes.
Also, there is no cost attached to it. You just need to do more coding. Remember - DTOs provide you much higher flexibility and loose coupling.
Ok that makes so much sense! And based off of your implementation, why are you using sequences for ID? Is this good practice, since a foreign key links to an Id? What if two calls are made concurrently to the database to post to users, wouldn't the foreign key of user_tenants be mismatched? Would it be better to use uuid?
That depends on implementation. I prefer id over uuid. Plus I need foreign key linkage as well. And I have not seen concurrency issues that much. But you can use uuid if you prefer.
@vikramkh can I close this issue now ? You can always raise new issue if you face any. BTW, I am in the middle of adding a use case of DTO pattern to this project. Hope that may help you further.
Yes. Thanks so much!
Hi! I just wondering how come there is no way to update the user_tenants table when a new user is added? Shouldn't it be updated with the addition of a user? Should we manually add a controller to do this or am I missing something? Because how would the program know what permissions the user has.