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.

Sign up for GitHub

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

Fixed #1750: Unexpected behavior when updation of talawa admin members profile. #1762

Closed
wants to merge 9 commits into from
Closed

Fixed #1750: Unexpected behavior when updation of talawa admin members profile. #1762

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
17f9cdb
fixed updation of talawa admin members profile and app language code
Devesh326 Jan 26, 2024
7b63184
Merge branch 'PalisadoesFoundation:develop' into develop
Devesh326 Jan 28, 2024
46961bc
Merge branch 'PalisadoesFoundation:develop' into develop
Devesh326 Jan 29, 2024
3f6b789
Merge branch 'develop' into develop
SiddheshKukade Feb 18, 2024
b6adacf
Merge branch 'develop' of https://github.com/PalisadoesFoundation/tal…
Devesh326 Feb 25, 2024
06e37f9
changed applangcode -> appLanguageCode
Devesh326 Feb 25, 2024
d151510
Merge branch 'develop' into develop
palisadoes Mar 3, 2024
3655736
Merge branch 'develop' into develop
palisadoes Mar 6, 2024
bbed653
Merge branch 'develop' into develop
palisadoes Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,12 +1368,14 @@ input UpdateOrganizationInput {

input UpdateUserInput {
address: AddressInput
appLanguageCode: String
birthDate: Date
educationGrade: EducationGrade
email: EmailAddress
employmentStatus: EmploymentStatus
firstName: String
gender: Gender
id: ID
lastName: String
maritalStatus: MaritalStatus
phone: UserPhoneInput
Expand Down
7 changes: 5 additions & 2 deletions src/resolvers/Mutation/updateUserProfile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
context,
) => {
const currentUser = await User.findOne({
_id: context.userId,
_id: args.data?.id || context.userId,
});

if (!currentUser) {
Expand Down Expand Up @@ -59,7 +59,7 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
// Update User
let updatedUser = await User.findOneAndUpdate(
{
_id: context.userId,
_id: args.data?.id || context.userId,
Copy link
Contributor

Choose a reason for hiding this comment

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

why would you allow anyone to update any user's profile they want to?

Copy link
Member

Choose a reason for hiding this comment

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

I think this api call would be done by the admin only.

Copy link
Contributor

Choose a reason for hiding this comment

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

this restriction isn't put in place, right now it allows any user to call this mutation

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be resolved. Only the profile's user, Admins and Super Admins must be able to edit profile settings.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, on it

},
{
$set: {
Expand Down Expand Up @@ -102,6 +102,9 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
firstName: args.data?.firstName
? args.data.firstName
: currentUser?.firstName,
appLanguageCode: args.data?.appLanguageCode
? args.data.appLanguageCode
: currentUser?.appLanguageCode,
gender: args.data?.gender ? args.data.gender : currentUser?.gender,
image: args.file ? uploadImageFileName : currentUser.image,
lastName: args.data?.lastName
Expand Down
2 changes: 2 additions & 0 deletions src/typeDefs/inputs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,8 @@ export const inputs = gql`
lastName: String
maritalStatus: MaritalStatus
phone: UserPhoneInput
appLanguageCode: String
id: ID
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the id field from here, the update will only be applied for a user with _id === context.userId

Copy link
Contributor

Choose a reason for hiding this comment

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

if this mutation is to be allowed to be triggered by both normal users and superadmins, some changes have to be made:-

  1. if args.input.id is not provided(it is null or undefined), the update will be carried out on the user with user._id equals to context.userId

  2. if args.input.id is provided, there are two scenarios:-

    1. if it's a normal user, context.userId must be equal to args.input.id
    2. if it's a superadmin there are no restrictions, they can update any user with user._id equals to args.input.id

some things to confirm with @palisadoes :-

  1. can one superadmin update other superadmin's user data?
  2. is this allowed only for superadmins, or admins of organizations too? the thing to remember is that a user is not tied to any one organization, they can be members/admins of many organizations simultaneously, this functionality doesn't exist in talawa-api right now because of existence of userType === "ADMIN" field on a user object, when in actuality the relationship between a user and an organization should exist on a junction between them, joining table in sql it is called

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. @xoldyckk is correct on the userType to organization relationship. This should be fixed in this issue.
    1. API: Fix the usage of userType #1711
  2. Senior usertypes should be able to edit or demote users of junior rank. They should be able to promote junior ranks to no more than their rank. Super Admins will need privileges to do this for everyone. I'm broadly basing this on Google's guidelines for their apps.
    1. https://apps.google.com/supportwidget/articlehome?hl=en&article_url=https%3A%2F%2Fsupport.google.com%2Fa%2Fanswer%2F1219251%3Fhl%3Den&assistant_event=welcome&assistant_id=usermasterbot&product_context=1219251&product_name=UnuFlow&trigger_context=a

Copy link
Author

Choose a reason for hiding this comment

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

Hey @xoldyckk,

I included the id in the mutation because during the API call to edit a specific user, in the backend context.userId only contained the id of the logged-in user. As a result, the profile being updated was that of the current user rather than the intended one. To address this, I passed the id of the user that requires editing/updating in the API call.

Following this logic, if args.input.id is null, it indicates the root user; otherwise, it's the id of the user whose profile needs updating.

Copy link
Member

Choose a reason for hiding this comment

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

ok. then it looks fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Devesh326 take some time and contemplate about what i said previously, the current logic as it stands allows a normal user to update user fields of any other user, they just need to pass in the other user's id in the input, frontend application can't prevent normal users from making these requests

understand the authentication/authorization flows, self review your code and possible edge cases, only then ask for reviews from other contributers

Copy link
Author

Choose a reason for hiding this comment

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

Yes noted.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

rename input UpdateUserInput to input UpdateUserProfileInput

make following changes for mutation:-

type UpdateUserProfilePayload {
  user: User
}

type Mutation {
  updateUserProfile(input: UpdateUserProfileInput!): UpdateUserProfilePayload
}

Copy link
Member

Choose a reason for hiding this comment

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

Please renamed to suit the conventions @Devesh326

Copy link
Author

Choose a reason for hiding this comment

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

Done


input UpdateUserPasswordInput {
Expand Down
2 changes: 2 additions & 0 deletions src/types/generatedGraphQLTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2238,12 +2238,14 @@ export type UpdateOrganizationInput = {

export type UpdateUserInput = {
address?: InputMaybe<AddressInput>;
appLanguageCode?: InputMaybe<Scalars['String']['input']>;
birthDate?: InputMaybe<Scalars['Date']['input']>;
educationGrade?: InputMaybe<EducationGrade>;
email?: InputMaybe<Scalars['EmailAddress']['input']>;
employmentStatus?: InputMaybe<EmploymentStatus>;
firstName?: InputMaybe<Scalars['String']['input']>;
gender?: InputMaybe<Gender>;
id?: InputMaybe<Scalars['ID']['input']>;
lastName?: InputMaybe<Scalars['String']['input']>;
maritalStatus?: InputMaybe<MaritalStatus>;
phone?: InputMaybe<UserPhoneInput>;
Expand Down
Loading