First attempt at effective name for admin mock #466

sfrunza13 · 2023-03-28T21:17:49Z

Fixes #306 or trying to anyways.

I found that I could not set multiple key value pairs on session to commit it so I made a new cookie instead, curious to see what people think.

humphd · 2023-03-28T21:29:34Z

app/session.server.ts

+export async function setEffectiveName(username: string, name: string): Promise<string> {
+ const user = await getUserByUsername(username);
+
+ if (user?.group.includes('admin')) {


Let's fix the User returned in https:/DevelopingSpace/starchart/blob/main/app/models/user.server.ts#L7-L21 and re-use/modify the code in https:/DevelopingSpace/starchart/blob/main/app/models/user.server.ts#L61-L77 to add properties to the User object that indicate if this is an admin.

User.isAdmin

This way we can do the /mycustomdomain(-dev)?-admins/.test(group) stuff to set it on the object.

Is making use of isAdmin when setting effectiveName enough or should I do as you said and make a boolean property on a user for whether they are Admin?

Nvm :) got it

Whatever you do, we need to put it on the User so we can pass that info around. But using isAdmin() like you are is good internally for your needs.

humphd · 2023-03-28T21:30:18Z

app/session.server.ts

+}
+
+export async function setEffectiveName(username: string, name: string): Promise<string> {
+ const user = await getUserByUsername(username);


If we don't have a user here, should we throw?

humphd · 2023-03-28T21:30:29Z

app/session.server.ts

+ return await effectiveNameCookie.serialize(name);
+ }
+
+ return await effectiveNameCookie.serialize(user?.username);


It seems odd to not have a user at this point.

right, I had tried to use getUser() before and this is a relic from that. We should have a user and if we don't I will throw.

humphd

Looking better, but I want to see us decorate the user object with more details. The front-end is going to need to know the correct username to use, whether this is an admin, etc via the User we return in https:/DevelopingSpace/starchart/blob/main/app/root.tsx#L34.

Please add this info to that User object so it can get passed around the app.

sfrunza13 · 2023-03-28T22:25:51Z

So it's ok to change the prisma schema for user?

humphd · 2023-03-28T22:26:40Z

You don't need to change the database, change the object we send back when you query it, same as I do with baseDomain which isn't in the db either.

humphd · 2023-03-28T22:38:22Z

app/models/user.server.ts

@@ -17,7 +18,7 @@ export async function getUserByUsername(username: PrismaUser['username']) {
 }

 // Decorate with the user's base domain as well
- return { ...user, baseDomain: buildUserBaseDomain(username) };
+ return { ...user, baseDomain: buildUserBaseDomain(username), isAdmin: isAdmin(username) };


Throw the effectiveUsername on here too. So we know:

are you an admin?

what is your real username? (who are you really)

what is your current effective username? (which user are you actually pretending to be)

I'm not sure how to do this since my effective user name is currently passed as a cookie from request to request on the header and we don't have access to the header in the getUserByUsername method.

humphd · 2023-03-29T00:21:48Z

app/models/user.server.ts

 }

-export async function getUserByUsername(username: PrismaUser['username']) {
+export async function getUserByUsername(username: PrismaUser['username'], request: Request) {


OK, I agree this is the wrong place to do this. Let's do it in the root.tsx loader.

On the server side, we'll read the username/effective username out of session. On client, we'll send it with the root on the loader.

So for client to use it I would make a utils method using useMathcesData() like we have for user?

The client would get it from the loader, which can access the request and also the db, and put all the data on the User object we return in root.tsx.

app/models/user.server.ts

humphd · 2023-03-29T15:43:38Z

app/root.tsx

@@ -30,8 +30,13 @@ export const links: LinksFunction = () => [
 ];

 export async function loader({ request, context }: LoaderArgs) {
+ const effectiveUsername = await getEffectiveName(request);
+ const userBeforeEffectiveName = await getUser(request);


Question: if both getEffectiveName and getUser both require request, can you not return the effectiveUsername on the result of getUser (i.e., you could do this internally in getUser)?

humphd · 2023-03-29T15:43:58Z

app/session.server.ts

@@ -36,6 +38,20 @@ export async function getUsername(request: Request): Promise<User['username'] |
 return username;
 }

+export async function getEffectiveName(request: Request): Promise<string> {


getEffectiveUsername

Also, change all effectiveName uses everywhere to effectiveUsername

humphd

Testing this locally, here is what I get on the client:

{
  "username": "user1",
  "displayName": "Johannes Kepler",
  "email": "[email protected]",
  "group": "mycustomdomain-faculty",
  "createdAt": "2023-03-28T17:18:24.557Z",
  "updatedAt": "2023-03-28T17:18:24.557Z",
  "deactivated": false,
  "baseDomain": "user1.starchart.com",
  "isAdmin": {},
  "effectiveUsername": null
}

According to the type of User, the effectiveUsername is string, but here we get null. Should we set the effectiveUsername to be the same as username if it's not set (e.g., regular user)?

Next, the isAdmin is an Object vs. a boolean.

humphd · 2023-03-29T22:44:24Z

app/session.server.ts

+ const user = await getUserByUsername(username);
+
+ if (user) {
+ if (user.isAdmin) {


if (user?.isAdmin) { return user; } throw redirect('/');

humphd · 2023-03-29T22:53:52Z

app/utils.ts

@@ -105,6 +105,26 @@ export function useUser(): User {
 return maybeUser;
 }

+export async function useEffectiveUser(): Promise<User> {


I think this should be:

useEffectiveUsername()

The user object will be identical. It's just that we're going to use this hook to get the specific "username" that the current user is using, which will either be:

user.effectiveUsername if the cookie is set and we're using someone else's username

user.username if the cookie is not set and we're using our own username

export async function useEffectiveUsername(): User['username'] { // The user must be logged in to call this. Use `useOptionalUser()` if you're not sure const { username, effectiveUsername } = useUser(); // Your username is either your real username, or the username you are temporarily using as an admin return effectiveUsername || username; }

I'm not sure how to go about this still, I have replaced what I had with what is suggested and it works for when we display it like on header instead of rendering a property of the user object I just render the effective username but we would have to have a different effective user object for the use of baseDomain for example. Or we could change how the method works so that getUserByUsername checks whether isAdmin and a non-empty effective username exist together and if that is the case will instead use the effectiveUsername when building domain. On second thought idk if this is possible because I don't think we have the extended User object in there since that is where we extend it in the first place. Maybe we just need to retrieve another effective user object like I was trying to do earlier?

humphd · 2023-03-29T22:54:22Z

app/models/user.server.ts

@@ -13,7 +13,7 @@ export interface User extends PrismaUser {
 username, admins (identified by group) will start with an empty string
 that can be changed to another users username on the admin portal for 
 them to impersonate */
- effectiveUsername: string;
+ effectiveUsername?: string;


The type here should be User['username'] so it matches what username is when we pass it around.

humphd · 2023-03-29T22:57:53Z

app/session.server.ts

+export async function getEffectiveUsername(request: Request): Promise<string> {
+ const cookie = request.headers.get('Cookie');
+ const effectiveUsername = await effectiveUsernameCookie.parse(cookie);
+ return effectiveUsername;


Let's return this like so:

return effictiveUsername as User['username'];

And you can drop the return type on the function.

humphd · 2023-03-29T22:58:37Z

app/session.server.ts

 if (username === undefined) {
 return null;
 }

 const user = await getUserByUsername(username);
 if (user) {
- return user;
+ return { ...user, effectiveUsername: effectiveUsername };


When the property name and value are the same, just use it once:

return { ...user, effectiveUsername };

humphd · 2023-03-29T22:59:03Z

app/session.server.ts

 export async function getUser(request: Request) {
 const username = await getUsername(request);
+ const effectiveUsername = await getEffectiveUsername(request);
 if (username === undefined) {


if (!username) {

app/models/user.server.ts

humphd · 2023-03-31T17:58:35Z

app/models/user.server.ts

+ username, admins (identified by group) will start with an empty string
+ that can be changed to another users username on the admin portal for 
+ them to impersonate */
+ effectiveUsername?: User['username'];


Question: if we're going to have a user and an effectiveUser (two different objects), is there any point having this value on the User type now? Should both user and effectiveUser be of type User?

I don't think we do need it since everything kind of comes from the cookie now, is there a better way of doing this than with the cookie? Maybe keeping track of it on this property we are thinking about removing, but this is dynamically appended every time we load same as baseDomain so that does not make sense. Anyways, I am ok with removing it I think.

Honestly it's kind of hard keeping track of stuff, and this isn't a big change either 😅

humphd · 2023-03-31T17:59:00Z

app/root.tsx

+ return json(
+ {
+ user: await getUser(request),
+ effectiveUser: await getEffectiveUser(request),


I would move your comment from User to this, so it explains what this is vs. user

humphd · 2023-03-31T17:59:18Z

app/root.tsx

+ }
+ // ,
+ // { headers: { 'Set-Cookie': await setEffectiveUsername('user3', 'user1') } }
+ // This is how I tested it, after refreshing once since user3 is admin it will mock user1


Can we remove this?

humphd · 2023-03-31T18:29:34Z

app/root.tsx

- );
+ return json({
+ user: await getUser(request),
+ /* the effectiveUsername, this is for admin impersonating users. Regular


Can you format this differently?

user: await getUser(request), /** * effictiveUser: ... * ... * ... */ effectiveUser: await ...

Also, effectiveUser vs. effectiveUsername.

humphd · 2023-03-31T18:29:59Z

app/session.server.ts

@@ -58,7 +59,7 @@ export async function getEffectiveUser(request: Request) {

 const user = await getUserByUsername(effectiveUsername);
 if (user) {
- return { ...user, effectiveUsername }; // should we have an effectiveUsername when mocking?
+ return user; // should we have an effectiveUsername when mocking?


You can drop this comment

humphd

I think this is right.

In a follow-up, we should add/update e2e tests for this stuff.

Does anything call useUser() now, or is it all useEffectiveUser()? Maybe the header will do it when an admin logs in and we want to show different usernames?

We should also file an issue to hook up the isAdmin stuff to the /admin route.

app/models/user.server.ts

sfrunza13 · 2023-03-31T19:08:19Z

Everything calls useEffectiveUser, if there is no effectiveUsername it will call getUser() in session server. As it stands there is no call to useUser(). I think we may be able to take it out of the root loader.

humphd · 2023-03-31T19:09:51Z

Everything calls useEffectiveUser, if there is no effectiveUsername it will call getUser() in session server. As it stands there is no call to useUser(). I think we may be able to take it out of the root loader.

OK, thanks for confirming. I think we'll still want it to show that an admin is impersonating another user, but probably in no other cases.

humphd · 2023-04-01T17:17:22Z

@sfrunza13 can you rebase and fix this conflict? Then we can get this in too!

sfrunza13 · 2023-04-01T19:48:30Z

yes, I will do this

humphd · 2023-04-01T22:14:04Z

app/session.server.ts

+export async function getEffectiveUser(request: Request) {
+ const username = await getUsername(request);
+ // get the effective username in the cookie at the time
+ const effectiveUsername = await getEffectiveUsername(request);


Run these at the same time:

const [username, effectiveUsername] = await Promise.all([getUsername(request), getEffectiveUsername(request)]);

humphd · 2023-04-01T22:14:36Z

app/session.server.ts

+ }
+
+ // if there is no effective username let the admin be himself
+ if (!effectiveUsername || effectiveUsername === '') {


It makes me nervous that effectiveUsername could be ''. When could this happen and why? Can we avoid that?

This was what I remembered from our discussion, admins would have an empty effectiveUsername until they select one

Can "empty" be undefined vs ''?

humphd · 2023-04-01T22:16:06Z

app/session.server.ts

+ return null;
+ }
+
+ // if there is no effective username let the admin be himself


I might update this wording slightly:

// if the user doesn't have an effective username, we're using their normal username (not impersonating)

humphd · 2023-04-01T22:17:26Z

app/session.server.ts

+
+ // if there is no effective username let the admin be himself
+ if (!effectiveUsername || effectiveUsername === '') {
+ return await getUser(request);


Here you use getUser() and in 69 you use getUserByUsername(). Can we use the same path in both cases? Not critical, but it feels odd that we have different code paths.

humphd · 2023-04-01T22:18:02Z

app/session.server.ts

@@ -62,12 +102,14 @@ export async function requireUsername(
 redirectTo: string = new URL(request.url).pathname
 ) {
 const username = await getUsername(request);
+ const effectiveUsername = await getEffectiveUsername(request);


Same comment as above, run at same time.

humphd · 2023-04-01T22:19:32Z

Also, please squash this down into 1 commit at this point.

humphd

This is great, thanks for persevering through the many reviews. I've left 2 final updates, and this is R+ from me.

humphd · 2023-04-02T12:37:03Z

app/session.server.ts

+
+export async function setEffectiveUsername(
+ username: string,
+ name: string | undefined


nit: you can use name?: string to mean "this is a string or undefined".

Should name really be called effectiveUsername? I think using name is ambiguous.

sfrunza13 · 2023-04-02T17:24:37Z

The above was some misclicks

sfrunza13 requested a review from humphd March 28, 2023 21:17

sfrunza13 self-assigned this Mar 28, 2023

humphd requested changes Mar 28, 2023

View reviewed changes

humphd requested changes Mar 29, 2023

View reviewed changes

Genne23v added this to the Milestone 0.8 milestone Mar 29, 2023

humphd requested changes Mar 29, 2023

View reviewed changes

humphd reviewed Mar 29, 2023

View reviewed changes

app/models/user.server.ts Show resolved Hide resolved

humphd mentioned this pull request Mar 30, 2023

ci: autogenerate release #269

Closed

humphd reviewed Mar 31, 2023

View reviewed changes

humphd requested changes Mar 31, 2023

View reviewed changes

sfrunza13 force-pushed the #306 branch from d59ebc6 to 062c6b9 Compare March 31, 2023 18:47

humphd previously approved these changes Mar 31, 2023

View reviewed changes

app/models/user.server.ts Show resolved Hide resolved

sfrunza13 dismissed humphd’s stale review via 84df9e1 April 1, 2023 00:59

humphd previously approved these changes Apr 1, 2023

View reviewed changes

Eakam1007 previously approved these changes Apr 1, 2023

View reviewed changes

sfrunza13 dismissed stale reviews from Eakam1007 and humphd via 628ac0b April 1, 2023 20:04

sfrunza13 force-pushed the #306 branch from 84df9e1 to 628ac0b Compare April 1, 2023 20:04

humphd requested changes Apr 1, 2023

View reviewed changes

Adding Admin Mocking by using a new cookie

cca8595

sfrunza13 force-pushed the #306 branch from a0d6db3 to cca8595 Compare April 2, 2023 02:13

humphd requested changes Apr 2, 2023

View reviewed changes

Changed naming in setEffectiveUsername

8fabedd

humphd approved these changes Apr 2, 2023

View reviewed changes

Genne23v modified the milestones: Milestone 0.8, Milestone 0.9 Apr 2, 2023

sfrunza13 requested a review from Eakam1007 April 2, 2023 17:03

Eakam1007 approved these changes Apr 2, 2023

View reviewed changes

sfrunza13 merged commit c6dc4b6 into DevelopingSpace:main Apr 2, 2023

sfrunza13 modified the milestones: Milestone 0.9, Milestone 0.8 Apr 2, 2023

First attempt at effective name for admin mock #466

First attempt at effective name for admin mock #466

Conversation

sfrunza13 commented Mar 28, 2023

Choose a reason for hiding this comment

sfrunza13 Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

sfrunza13 commented Mar 28, 2023

humphd commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

sfrunza13 commented Mar 31, 2023

humphd commented Mar 31, 2023

humphd commented Apr 1, 2023

sfrunza13 commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented Apr 1, 2023

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 commented Apr 2, 2023

sfrunza13 Mar 28, 2023 •

edited

Loading

sfrunza13 Mar 30, 2023 •

edited

Loading

sfrunza13 Mar 31, 2023 •

edited

Loading