Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User representations don't match #6299

Closed
ocket8888 opened this issue Oct 19, 2021 · 3 comments · Fixed by #6832
Closed

User representations don't match #6299

ocket8888 opened this issue Oct 19, 2021 · 3 comments · Fixed by #6832
Labels
authentication Relating to login, registration, passwords, tokens, etc. improvement The functionality exists but it could be improved in some way. medium difficulty the estimated level of effort to resolve this issue is medium medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Ops related to Traffic Ops

Comments

@ocket8888
Copy link
Contributor

ocket8888 commented Oct 19, 2021

This Improvement request (usability, performance, tech debt, etc.) affects these Traffic Control components:

  • Traffic Ops
  • Documentation

Current behavior:

The representations of Users are different depending on the API endpoint used to view them, e.g. /user/current uses the field roleName to hold the name of the User's Role, while /users shows the same information in the rolename property.

The current model for users (API v3.0, 4.0 makes some structural changes in some contexts) is such that requests and responses have different "shapes" depending on the request method - and some fields are even renamed between requests and responses.

A TypeScript model of Users in APIv3
export interface PostRequestUser {
	addressLine1?: string | null;
	addressLine2?: string | null;
	city?: string | null;
	company?: string | null;
	confirmLocalPasswd: string;
	country?: string | null;
	email: string;
	fullName: string;
	gid?: number | null;
	id?: never;
	lastUpdated?: never;
	localPasswd: string;
	newUser?: boolean | null;
	phoneNumber?: string | null;
	postalCode?: string | null;
	publicSshKey?: string | null;
	registrationSent?: never;
	role: number;
	roleName?: never;
	rolename?: never;
	stateOrProvince?: string | null;
	tenant?: never;
	tenantId?: never;
	tenantID: number;
	uid?: number | null;
	username: string;
}

interface PutRequestNotChangingPasswordUser {
	addressLine1?: string | null;
	addressLine2?: string | null;
	city?: string | null;
	company?: string | null;
	confirmLocalPasswd?: never;
	country?: string | null;
	email: string;
	fullName: string;
	gid?: number | null;
	id?: never;
	lastUpdated?: never;
	localPasswd?: never;
	newUser?: boolean | null;
	phoneNumber?: string | null;
	postalCode?: string | null;
	publicSshKey?: string | null;
	registrationSent?: never;
	role: number;
	roleName?: never;
	rolename?: never;
	stateOrProvince?: string | null;
	tenant?: never;
	tenantId?: never;
	tenantID: number;
	uid?: number | null;
	username: string;
}

export type PutRequestUser = PostRequestUser | PutRequestNotChangingPasswordUser;

interface ResponseUser {
	addressLine1: string | null;
	addressLine2: string | null;
	city: string | null;
	company: string | null;
	country: string | null;
	email: string;
	fullName: string;
	gid: number | null;
	id: number;
	lastUpdated: Date;
	newUser: boolean | null;
	phoneNumber: string | null;
	postalCode: string | null;
	publicSshKey: string | null;
	registrationSent?: null | Date;
	role: number;
	stateOrProvince: null;
	tenant: string;
	tenantID?: never;
	tenantId: number;
	uid: number | null;
	username: string;
}

export interface PutOrPostResponseUser extends ResponseUser {
	/**
	 * This appears only in response to POST requests, or to PUT requests where
	 * the user's password was changed.
	 */
	confirmLocalPasswd?: string;
	rolename?: never;
	roleName: string;
}

export interface GetResponseUser extends ResponseUser {
	confirmLocalPasswd?: never;
	rolename: string;
	roleName?: never;
}

export type User = PutRequestUser | PostRequestUser | PutOrPostResponseUser | GetResponseUser;

export interface ResponseCurrentUser {
	addressLine1: string | null;
	addressLine2: string | null;
	city: string | null;
	company: string | null;
	country: string | null;
	email: string;
	fullName: string;
	gid: number | null;
	id: number;
	lastUpdated: Date;
	localUser: boolean;
	newUser: boolean;
	phoneNumber: string | null;
	postalCode: string | null;
	publicSshKey: string | null;
	role: number;
	roleName: string;
	stateOrProvince: string | null;
	tenant: string;
	tenantId: number;
	uid: number | null;
	username: string;
}

export interface RequestCurrentUser {
	addressLine1?: never;
	addressLine2?: never;
	city?: never;
	company?: never;
	country?: never;
	email?: never;
	fullName?: never;
	gid?: never;
	id?: never;
	lastUpdated?: never;
	localUser?: never;
	newUser?: never;
	phoneNumber?: never;
	postalCode?: never;
	publicSshKey?: never;
	role?: never;
	roleName?: never;
	stateOrProvince?: never;
	tenant?: never;
	tenantId?: never;
	uid?: never;
	username?: never;
}

export type CurrentUser = ResponseCurrentUser | RequestCurrentUser;
  • requests use tenantID, responses have tenantId
  • responses from POST requests and PUT requests where the password was changed include confirmLocalPasswd but GET representations never do
  • POST and PUT responses don't include registrationSent but GET requests always do
  • /users never shows localUser, but /user/current always does
  • POST and PUT requests allow newUser to be null, but that's actually not allowed in the database, and GET requests will always show an actual boolean value. Plus, if newUser was null in a POST or PUT to /users, it will be null in the response, but if it was null in a PUT to /user/current the response will properly show false like a subsequent GET
  • PUT to /user/current doesn't require any fields, not even those required by POST and PUT requests to /users, making it non-idempotent in violation of the HTTP spec
  • PUT to /user/current literally doesn't work at all, see PUT /user/current doesn't work #6367

and then of course we have the pervasive issue where things are allowed to be missing from the request as a synonym for "null", but that's more an issue just because in TS/JS undefined and null are different, but Go makes no such distinction without jumping through some encoding/json.RawMessage hoops.

New behavior:

Users should be represented consistently in a single way throughout the API.

@ocket8888 ocket8888 added Traffic Ops related to Traffic Ops improvement The functionality exists but it could be improved in some way. labels Oct 19, 2021
@rawlinp
Copy link
Contributor

rawlinp commented Oct 19, 2021

Is roleName still a field in /users as of the 4.0 users+permissions work? That is, is this issue fixed once pre-4.0 APIs are removed?

@ocket8888
Copy link
Contributor Author

Yes and no. I think you might be right about that specific field, but they still use different structs because of weird, obscure differences between the two. I think uid and guid show up in one when they're null but not the other, maybe the "current" user model includes login tokens? I honestly can't remember, but until they're using the same struct I don't feel safe calling them the same. Which, if they were, should not be using two different structs, even if that's true.

@ocket8888
Copy link
Contributor Author

Update: I added a model of Users which I think illustrates all of the problems. It's APIv3, so Role stuff has been fixed in the latest version.

@ocket8888 ocket8888 added medium difficulty the estimated level of effort to resolve this issue is medium medium impact impacts a significant portion of a CDN, or has the potential to do so labels Dec 1, 2021
@zrhoffman zrhoffman added the authentication Relating to login, registration, passwords, tokens, etc. label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication Relating to login, registration, passwords, tokens, etc. improvement The functionality exists but it could be improved in some way. medium difficulty the estimated level of effort to resolve this issue is medium medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants