Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3916: Authentication for media #3916
MSC3916: Authentication for media #3916
Changes from 17 commits
7606e53
3076de0
55303b5
d601637
c2ae25e
8351ebe
106ce55
a14c4af
92eba2c
a76d97f
8bb5159
e1e8a6a
71b8db4
1c864a3
e5c9316
41d2aa2
d73025b
656dfb8
87c08e0
1fe8d71
aac1909
0b4f2c9
f1777c2
4fe23e5
db90b92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers: Please compare this MSC against #2461 , which is proposed for rejection/closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turt2live I think we can resolve this since #2461 is closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's still useful to compare the MSCs, given the ideas and concepts of 2461 are meant to be incorporated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's easier for a reader to go from the PR to the doc than the other way around, so could we do:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR doesn't show any changes to accepted MSCs, unfortunately. Ideally this would be a link to the spec, but unstable has the same longevity problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really true? What data do you have to back this up? It seems like a fairly elegant solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not hard data, but for the last 6 years it's felt like every third user for Element Web has disabled these things and more :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, the latter seems spurious since cookies and localstorage will almost always be enabled/disabled as a pair, and Element has been relying on localstorage for years. The former is true in that setting a cookie for the homeserver would be a little weird. In my mind, the pros / cons are:
I'm not going to make this a concern as I don't think it's blocking, but it might be worth a quick sanity check to make sure we really are committing to the right thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still planning to fast-follow with MSC3911, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, yes. It won't be in the same spec release though.