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

MSC2787 Add support for splitting UPK IDs #301

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

brianathere
Copy link

Add code coverage for SplitID. Create SplitIDWithSigil for future extension. Mark SplitID deprecated as callers should only pass ID with embedded sigals, letting SplitIDWithSigil parse the sigil.

Pull Request Checklist

Signed-off-by: Brian Meek <[email protected]>

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

I have a few problems with this:

  • the names of the functions are misleading and unclear. SplitIDWithSigil doesn't actually take a sigil and SplitID says the sigil is unused but it in fact is for validation.
  • The newer now recommended function is less secure and less correct compared to the deprecated function. We use SplitID when we know what kind of ID we're supposed to have e.g. a user ID. Specifying the @ in the function signature as a sigil ensured that you can't send a room alias as a user ID for example. Now the newer "recommended" form doesn't do this critical check.

I don't think this overall improves things. You've been rather prescriptive about the API with insufficient justifications why this is better. I imagine you're doing this because GMSL auth code checks using SplitID for well-formed IDs which for this MSC you cannot know upfront what the sigil will be? The answer then isn't to make us do less validation: specify which sigils you expect to see and ensure that one of them is present. Furthermore, you need to be careful that this is only going to apply in room versions / other flags which enable this: we cannot have UPKs being treated as valid everywhere.

@brianathere
Copy link
Author

I have a few problems with this:

  • the names of the functions are misleading and unclear. SplitIDWithSigil doesn't actually take a sigil and SplitID says the sigil is unused but it in fact is for validation.
  • The newer now recommended function is less secure and less correct compared to the deprecated function. We use SplitID when we know what kind of ID we're supposed to have e.g. a user ID. Specifying the @ in the function signature as a sigil ensured that you can't send a room alias as a user ID for example. Now the newer "recommended" form doesn't do this critical check.

I don't think this overall improves things. You've been rather prescriptive about the API with insufficient justifications why this is better. I imagine you're doing this because GMSL auth code checks using SplitID for well-formed IDs which for this MSC you cannot know upfront what the sigil will be? The answer then isn't to make us do less validation: specify which sigils you expect to see and ensure that one of them is present. Furthermore, you need to be careful that this is only going to apply in room versions / other flags which enable this: we cannot have UPKs being treated as valid everywhere.

Great feedback, I agree with it not accomplishing anything (this was setting up for a change in the Dendrite server using UPK), and that it weakens the contract for the caller not declaring what type of ID. I'll revise for the latter.

As for talking this over, is there a forum you'd suggest other than in Github comments?

@kegsay
Copy link
Member

kegsay commented Apr 8, 2022

#dendrite:matrix.org or #dendrite-dev:matrix.org are good candidates. We can support your changes more there.

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.

2 participants