Hide user avatar for blocked and unconfirmed users
What does this MR do and why?
Follow-up to #341325 (closed) and !75032 (merged) (merged).
We should mask the user avatar for blocked or unconfirmed users to avoid it being used for spam. You can see in screenshots below this also masks the Gravatar for a user, so they can't even show spam via that external service when they're blocked in GitLab.
Admin users are always able to see the user avatar regardless of status.
Screenshots or screen recordings
Before
After
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %14.6
assigned to @dblessing
mentioned in issue #347134 (closed)
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Diogo Frazão ( @dfrazao-gitlab
) (UTC+1, 7 hours ahead of@dblessing
)Sean McGivern ( @smcgivern
) (UTC+0, 6 hours ahead of@dblessing
)test Quality for spec/features/*
Andrejs Cunskis ( @acunskis
) (UTC+2, 8 hours ahead of@dblessing
)Maintainer review is optional for test Quality for spec/features/*
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded 1 commit
- c4eba128 - Hide user avatar for blocked and unconfirmed users
Allure report
allure-report-publisher
generated test report for f2a83cd8!review-qa-smoke:
test reportSetting label(s) ~"Category:Authentication and Authorization" sectiondev based on ~"group::access".
added sectiondev + 1 deleted label
@dblessing, please can you answer the question: Should this have a feature flag? to help with code review for the Access group.This nudge was added by this triage-ops policy.
- Resolved by Sean McGivern
It would be useful for admins to see the avatar still when a user is blocked or unconfirmed. Can this be configured to hide the avatar for all users other than admins?
This will help us better determine if a user should be blocked for unconfirmed users. For Blocked accounts, as we use a lot of automation, it will help us better assess whether the block was a false positive or not.
This may have already been handled in this MR; apologies if so, but I don't have the skills to understand all the changes.
added 1 commit
- 54c841d7 - Hide user avatar for blocked and unconfirmed users
added 1 commit
- 2b822773 - Hide user avatar for blocked and unconfirmed users
added 1 commit
- 92c74714 - Hide user avatar for blocked and unconfirmed users
added pipeline:skip-undercoverage label
added 757 commits
-
92c74714...c60c6ed3 - 756 commits from branch
master
- 25274ea7 - Hide user avatar for blocked and unconfirmed users
-
92c74714...c60c6ed3 - 756 commits from branch
added 1 commit
- e54c27cc - Hide user avatar for blocked and unconfirmed users
- Resolved by Sean McGivern
@alberts-gitlab Do you mind doing an initial backend review, please?
requested review from @acunskis and @alberts-gitlab
- Resolved by Drew Blessing
- Resolved by Drew Blessing
@dblessing I just have 2 minor suggestions. Otherwise looks good to me.
Feel free to pass to maintainer after addressing them.
removed review request for @alberts-gitlab
@alberts-gitlab
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
removed review request for @acunskis
added 1 commit
- 6c995198 - Hide user avatar for blocked and unconfirmed users
requested review from @smcgivern
added 1 commit
- f2a83cd8 - Hide user avatar for blocked and unconfirmed users
- Resolved by Sean McGivern
@dblessing @alberts-gitlab thanks, this looks good to me. I'll merge once #347498 (closed) is fixed.
enabled an automatic merge when the pipeline for 661aedf4 succeeds
mentioned in commit 8e8e9cb6
added workflowstaging-canary label
mentioned in merge request !76888 (merged)
added workflowstaging label and removed workflowstaging-canary label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
mentioned in commit ee53b29d
mentioned in merge request !76935 (merged)
mentioned in issue gitlab-com/gl-infra/production#6083 (closed)
mentioned in issue #348698 (closed)
@dblessing can you please take a look at this bug #348698 (closed)? It appears this change introduced 500 error in some Blame requestsadded releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in merge request !77250 (merged)
mentioned in merge request kubitus-project/kubitus-installer!474 (merged)
Late to the party... but has it been considered to make this configurable?
It's a bit sad to just see the grayed-out avatar for a person who has left the company (and therefore is blocked in our GitLab) when looking at list of commits for older branches in our repositories. To be frank, it's a bit disturbing to look at actually since it makes their commits look very different than all others, for little reason.I can get the "spam" part though, particularly with Gravatar, but it would be nice to not force this hiding of avatars for all users in all installations unconditionally. I didn't know about this "feature" so I thought the problem was that the person's avatar was missing in GitLab... updated it, still didn't work, so I thought it was a caching issue.
(Is the problem that we shouldn't block users who have left the company, but disable their accounts in some other way? We use LDAP for authentication.)
Edited by Per Lundberg/cc @dblessing
@perlun Sure, we can consider it. Can you please create an issue? Then we'll send it to the groupanti-abuse group for consideration.
Thanks. Issue created, #424093. Let's continue there @dblessing. I added the groupanti-abuse label to it but feel free to relabel/edit as you please.
mentioned in issue #424093