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

Feature 2825/adjust admin menu #2828

Merged
merged 13 commits into from
Jan 23, 2025
Merged

Conversation

ocielliottc
Copy link
Collaborator

No description provided.

@ocielliottc ocielliottc linked an issue Jan 20, 2025 that may be closed by this pull request
if (!currentUserServices.isAdmin()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

UUID kudosId = kudos.getId();
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need to replace this with a permission check? I don't see it happening elsewhere in here, but I could be missing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The controller handles this:

    @Put
    @RequiredPermission(Permission.CAN_ADMINISTER_KUDOS)
    public Kudos approve(@Body @Valid Kudos kudos) {
        return kudosServices.approve(kudos);
    }

if (!currentUserServices.isAdmin()) {
throw new PermissionException(NOT_AUTHORIZED_MSG);
}

Kudos kudos = kudosRepository.findById(id).orElseThrow(() ->
Copy link
Member

Choose a reason for hiding this comment

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

Same above here. Is there a permission check happening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with the delete:

    @Delete("/{id}")
    @Status(HttpStatus.NO_CONTENT)
    @RequiredPermission(Permission.CAN_ADMINISTER_KUDOS)
    public void delete(@NotNull UUID id) {
        kudosServices.delete(id);
    }


MemberProfile currentUser = currentUserServices.getCurrentUser();
boolean isAdmin = currentUserServices.isAdmin();
if (!isAdmin && (currentUser == null || !currentUser.getId().equals(memberProfile.getId()))) {
Copy link
Member

Choose a reason for hiding this comment

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

Was this an intentional or accidental can kicking? 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, intentional in that there are no required permissions to update a member profile. Since every member can update their own profile, I didn't see a way to limit which profile could be updated except by using the existing mechanism of checking for admin. But, as I'm writing this, I'm thinking that we could add a permission to be able to "update all profiles" and specifically check for it here instead of admin. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think that would work!

Copy link
Member

Choose a reason for hiding this comment

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

👍 I'm onboard with that.


@Override
@CacheInvalidate(cacheNames = {"member-cache"})
public MemberProfile updateCurrentUserProfile(MemberProfile memberProfile) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be misleading. This isn't related to the "current user" at all, is it? Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it is only called from the current user controller when updating the "last seen" for the current user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it should be renamed to something like "directProfileUpdate" or something indicating that there is no security related to it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we isolate that code to update the "last seen" here and call it something like 'updateLastSeen' and just pass in the user id? that feels less risky than an unsecured profile update API.

@mkimberlin mkimberlin merged commit 8b9d3e7 into develop Jan 23, 2025
5 checks passed
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.

Adjust ADMIN menu to use permissions
2 participants