Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix application service not being able to join remote federated room without a profile set #13131

Merged

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jun 29, 2022

Fix application service not being able to join remote federated room without a profile set

Fix #4778

Complement tests: matrix-org/complement#399

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@MadLittleMods MadLittleMods added A-Application-Service Related to AS support T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Jun 29, 2022
"Failed to get profile information while processing remote join for %r: %s",
target,
e,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same pattern that we already use in

try:
if "displayname" not in content:
displayname = await profile.get_displayname(target)
if displayname is not None:
content["displayname"] = displayname
if "avatar_url" not in content:
avatar_url = await profile.get_avatar_url(target)
if avatar_url is not None:
content["avatar_url"] = avatar_url
except Exception as e:
logger.info(
"Failed to get profile information for %r: %s", target, e
)

@MadLittleMods MadLittleMods changed the title Application service can join remote federated room without profile set Fix application service not being able to join remote federated room without a profile set Jun 29, 2022
profile = self.profile_handler
if not content_specified:
content["displayname"] = await profile.get_displayname(target)
content["avatar_url"] = await profile.get_avatar_url(target)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jun 29, 2022

Choose a reason for hiding this comment

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

Feels like these profile functions would be better if they just returned None when there is no profile for the local user. Profiles aren't required.

  • async def get_displayname(self, target_user: UserID) -> Optional[str]:
    if self.hs.is_mine(target_user):
    try:
    displayname = await self.store.get_profile_displayname(
    target_user.localpart
    )
    except StoreError as e:
    if e.code == 404:
    raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND)
    raise
    return displayname
    else:
    try:
    result = await self.federation.make_query(
    destination=target_user.domain,
    query_type="profile",
    args={"user_id": target_user.to_string(), "field": "displayname"},
    ignore_backoff=True,
    )
    except RequestSendFailed as e:
    raise SynapseError(502, "Failed to fetch profile") from e
    except HttpResponseException as e:
    raise e.to_synapse_error()
    return result.get("displayname")
  • async def get_avatar_url(self, target_user: UserID) -> Optional[str]:
    if self.hs.is_mine(target_user):
    try:
    avatar_url = await self.store.get_profile_avatar_url(
    target_user.localpart
    )
    except StoreError as e:
    if e.code == 404:
    raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND)
    raise
    return avatar_url
    else:
    try:
    result = await self.federation.make_query(
    destination=target_user.domain,
    query_type="profile",
    args={"user_id": target_user.to_string(), "field": "avatar_url"},
    ignore_backoff=True,
    )
    except RequestSendFailed as e:
    raise SynapseError(502, "Failed to fetch profile") from e
    except HttpResponseException as e:
    raise e.to_synapse_error()
    return result.get("avatar_url")

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree. Pretty much everywhere uses the same pattern as here, and we already return None if the user sets and then deletes their display name

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Happy for this to land as is, or to refactor calls to the profile store

@MadLittleMods MadLittleMods merged commit 2c2a42c into develop Jul 5, 2022
@MadLittleMods MadLittleMods deleted the madlittlemods/no-profile-necessary-to-remote-join-as-as branch July 5, 2022 10:56
@MadLittleMods
Copy link
Contributor Author

Will merge for now ⏩

Thanks for the review @erikjohnston 🐹

MadLittleMods added a commit to matrix-org/complement that referenced this pull request Jul 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Application-Service Related to AS support T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application Service user cannot join rooms on other homeservers without account on origin homeserver
2 participants