Skip to content

Conversation

@galenguyer
Copy link
Member

hehe sso go brr

@galenguyer galenguyer requested a review from Mstrodl March 29, 2022 21:55
Copy link
Member

@Mstrodl Mstrodl left a comment

Choose a reason for hiding this comment

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

Looks very good, just a few considerations because I want to keep gatekeeper responses fast

const fetch = (await fetchPromise).default;

const resp = await fetch(
"https://sso.csh.rit.edu/auth/realms/csh/protocol/openid-connect/auth?client_id=gatekeeper&response_type=token&response_mode=fragment&redirect_uri=https%3A%2F%2Fgatekeeper.csh.rit.edu%2Fcallback",
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool if this didn't always use the same client ID (think: audiophiler)

Comment on lines +119 to +136
const response = {};
for (const attribute of user.attributes) {
if (attribute.type == "jpegPhoto") {
response[attribute.type] = attribute._vals[0].toString("base64");
} else {
const values = attribute._vals.map((value) => value.toString("utf8"));
if (ARRAYS.has(attribute.type)) {
response[attribute.type] = values;
} else {
if (values.length > 1) {
console.warn(`${attribute.type} has many values!!`);
}
response[attribute.type] = values.join(",");
}
}
}

const uid = response["uid"];
Copy link
Member

Choose a reason for hiding this comment

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

Having my jetbrains arc

💡 Loop can be simplified:

const uid = user.attributes.find(attribute => attribute.type == "uid")._vals[0].toString("utf8");

We should really use a different client for drink vs normal... One should grant read/write drink credits scope to /drink but no others. req.associationType will give you a hint for this.

Copy link
Member

Choose a reason for hiding this comment

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

Disappointed there's no way to search by ipaUniqueID on keycloak... 😢

Comment on lines +102 to +109
const userDocument = await req.ctx.db.collection("users").findOne({
id: {$eq: key.userId},
disabled: {$ne: true},
});
if (!userDocument) {
res.status(404).json({message: "User not found or disabled"});
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary query to DB


const uid = response["uid"];

res.json(await impersonate.getImpersonationToken(uid));
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we added ipaUniqueID to the response too, I really want to encourage people to use that attribute where possible


let user;
try {
user = await findUser(key.userId);
Copy link
Member

Choose a reason for hiding this comment

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

This pulls every attribute, which will be MUCH slower than just fetching attributes we want. Look at findUser in routes/users.js, notice how it only enumerates a few attributes we care about. I would make another function that just fetches a uid from a ipaUniqueID... It should be much faster

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add an optional attributes parameter to findUser? Would be nice if that got moved out into another file because I was being lazy and stuck the same function in routes/users.js and routes/memberProjects.js

npm-error.log
/.vscode
/.idea
.env
Copy link
Member

Choose a reason for hiding this comment

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

/.env

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.

2 participants