PIMS-669: User & Roles Services #2037
Conversation
… fully implement the join tables between users-roles, users-agencies, and roles-claims, allowing easy manipulation in the query builders
…till need to clarify a few things.
… the controller itself.
…ned by the service. Also adjusted some instances of kc user to internal user conversion to use 'preferred_username' instead of idir or bceid username.
GrahamS-Quartech
requested review from
LawrenceLau2020,
Sharala-Perumal,
dbarkowsky and
TaylorFries
as code owners
|
There's a lot to look through in terms of review here, but some particular things I'd appreciate a second set of eyes on:
|
There was a problem hiding this comment.
There's a few things that I think we need to meet as a group about and discuss what we want out of these services/controllers for standarization.
- I think we need a way to determine which HTTP codes to send back when a service fails. At the moment, lots of them will throw a generic 400 it seems. This is the custom error I've been throwing in the other PR to try and deal with that:
export class ErrorWithCode extends Error {
public code: number;
constructor(message: string, code?: number) {
super(message);
this.code = code;
}
}Then this could be caught in the controller and returned with a custom code/message.
- What's the easiest way to handle typing with interfaces vs types made from TypeORM? Seems like there are probably going to be conversions, so maybe there's a way to organize these.
Also, to answer some questions from your other comment:
- I think the KeycloakId might be useless anyway. We don't seem to actually use it anywhere.
- Logic on the sync functions looks good. We'll know for sure after we try running it.
- There's a
usernameproperty that comes back when searching for users in Keycloak. I suspect it's this, because the ones in the database end with@idirtoo.
{
"data": [
{
"username": "fohe4m5pn8clhkxmlho33sn1r7vr7m67@idir",
"email": "julius-caesar@email.com",
"firstName": "Julius",
"lastName": "Caesar",
"attributes": {
"display_name": [
"Julius Caesar"
],
"idir_user_guid": [
"fohe4m5pn8clhkxmlho33sn1r7vr7m67"
],
"idir_username": [
"JULIUSCA"
]
}
}
]
}
Yeah I think this is good, we should probably handle it this way.
Well, we can probably just use the generated typeorm types in most cases right? Though I guess there may be times we need a different shape to the object.
This is the same format as the preferred_username that the keycloak services are returning, so I think that's the right one to use then. |
…d with whatever extra columns are necessary. It is also possible to easily join from either side of the relation.
…t found. Fixed some of the schemas again so that users can only have one agency at a time instead of multiple.
There was a problem hiding this comment.
I think there's still a disconnect about where the user roles should be grabbed from. In the original API, they are coming from the database it looks like, not directly from Keycloak. What should our source of truth be? If it's Keycloak, what are the benefits to even storing the role mappings in the database? This affects functions like this:
// How you interpreted it
const getKeycloakRoles = async () => {
const roles = await KeycloakService.getKeycloakRoles();
return roles.map((a) => a.name);
};
// How I interpreted it
const getUserRoles = async (username: string) => {
return await AppDataSource.getRepository(UserRoles).find({
where: {
User: {
Username: username,
},
},
relations: {
Role: true,
},
});
};| const decodeJWT = (jwt: string) => { | ||
| try { | ||
| return JSON.parse(Buffer.from(jwt, 'base64').toString('ascii')); | ||
| } catch { | ||
| throw new Error('Invalid input in decodeJWT()'); | ||
| } | ||
| }; | ||
|
|
||
| if (!req.token) return res.status(400).send('No access token'); | ||
| const [header, payload] = req.token.split('.'); | ||
| if (!header || !payload) return res.status(400).send('Bad token format.'); | ||
|
|
||
| const info = { | ||
| header: decodeJWT(header), | ||
| payload: decodeJWT(payload), | ||
| }; | ||
|
|
||
| if (info) { | ||
| return res.status(200).send(info.payload); | ||
| } else { | ||
| return res.status(400).send('No keycloak user authenticated.'); | ||
| } |
There was a problem hiding this comment.
We might be able to do away with this. If we just want user info, it should be available under the res.user object in the backend and state.userinfo in the frontend.
There was a problem hiding this comment.
The user info contained in user doesn't have the same fields as this as far as I can tell, but I agree that this seems like something we could do away with. Not really sure why we would need to expose this.
| }; | ||
|
|
||
| const getRoleById = async (roleId: UUID) => { | ||
| return AppDataSource.getRepository(Roles).findOne({ |
There was a problem hiding this comment.
| return AppDataSource.getRepository(Roles).findOne({ | |
| return await AppDataSource.getRepository(Roles).findOne({ |
| if (!existing) { | ||
| throw new ErrorWithCode('Role was not found.', 409); | ||
| } | ||
| const retRole = AppDataSource.getRepository(Roles).remove(role); |
There was a problem hiding this comment.
| const retRole = AppDataSource.getRepository(Roles).remove(role); | |
| const retRole = await AppDataSource.getRepository(Roles).remove(role); |
…me issues with the filtering bodies on admin users and roles
| @Index({ unique: true }) | ||
| KeycloakUserId: string; | ||
|
|
||
| @Column({ name: 'AgencyId', type: 'varchar', length: 6 }) |
There was a problem hiding this comment.
Can we make the AgencyId column nullable to accommodate the users in PIMS that have no agencies?
@Column({ name: 'AgencyId', type: 'varchar', length: 6, nullable: true })
🎯 Summary
PIMS-669
import type { }must be used to avoid circular dependencies when defining many to many relations in different files.🔰 Checklist