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

Matrix Badges - "guests not allowed" #10776

Closed
cyb3rko opened this issue Dec 30, 2024 · 15 comments
Closed

Matrix Badges - "guests not allowed" #10776

cyb3rko opened this issue Dec 30, 2024 · 15 comments
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas

Comments

@cyb3rko
Copy link
Contributor

cyb3rko commented Dec 30, 2024

Are you experiencing an issue with...

shields.io

🐞 Description

The badges for Matrix rooms all show the same error, "guests not allowed".

It started coming up a few weeks ago I would say.
I've noticed it on my repo: https://github.com/cyb3rko/flashdim

But here are some more examples:

🔗 Link to the badge

https://img.shields.io/matrix/flashdim%3Amatrix.org?logo=matrix&label=Matrix%20Chat&color=black

💡 Possible Solution

No response

@cyb3rko cyb3rko added the question Support questions, usage questions, unconfirmed bugs, discussions, ideas label Dec 30, 2024
Copy link
Contributor

Badge tested using npm run badge https://img.shields.io/matrix/flashdim%3Amatrix.org?logo=matrix&label=Matrix%20Chat&color=black
Output is available here

@chris48s
Copy link
Member

Looking at the examples you've posted, these are all for rooms hosted on matrix.org
It looks like matrix.org has disabled guest access, which was the method we were using to get stats.

This isn't the case for all matrix servers though. For example, these badges (using different matrix servers) work just fine

although I am aware that matrix.org is by far the most common choice of server so this upstream change breaks the majority matrix badges.

In general, the method the matrix badges use to get stats is not very good for performance reasons outlined in #10138 even where guest access is available. Moving to the solution outlined in that issue would solve this problem and also provide performance benefits. One of the reasons nobody worked on that is because it is a bit fiddly to deal with different matrix servers out there running different versions of the software. I wonder if enough time has passed that we could simplify the implementation by just dropping the old guest access method and say you now must have the summary endpoint to use the badge. That would simplify things a bit.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Dec 31, 2024

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Dec 31, 2024

But then again I wonder how https://app.element.io/#/room/#flashdim:matrix.org fetches the user number without the summary call

@chris48s
Copy link
Member

OK. So just having done a bit more reading on it, it looks like:

  • MSC3266: Room summary API matrix-org/matrix-spec-proposals#3266 is still not accepted
  • This feature is off by default in Synapse (which I think seems to be the most common server implementation?). You have to explicitly opt-in to an experimental feature to enable this endpoint.
  • This proposal is not implemented in some server implementations
  • I've not done a full census, but having checked a few other servers I could find based on searching for badges in public READMEs, I have not been able to find another server other than matrix.org with the summary endpoint enabled.

Given that, I think it is too soon to drop the existing method and require the summary endpoint. Doing that would probably fix badges for matrix.org users but break it for a lot of other users. Annoyingly, I think we would ideally need to able to use both methods at this stage.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Dec 31, 2024

Someone on Matrix mentioned that apparently shields.io got blocked on matrix.org:

https://matrix.to/#/!QQpfJfZvqxbCfeDgCj:matrix.org/$NzO5dGBTC0TjhDMlZgL36KTt4NWIQU2VT8rbbcaZmcg?via=matrix.org&via=envs.net&via=element.io


Furthermore, guest access doesn't seem to be disabled for matrix.org

@chris48s
Copy link
Member

That's exciting.

Matrix badges are cached downstream for 30 seconds

static _cacheLength = 30

but we could, of course, increase that.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Dec 31, 2024

It should be increased a lot I feel like.
The only thing fetched in the end is the number of users, right?

If so, we could easily say a cache of 1 day would still be enough. I mean the number of users is not a sensitive stat that has to be up-to-date permanently.

@chris48s
Copy link
Member

Yeah looking back at it, I think the person who originally contributed this probably copied this from the discord badge, which shows the number of users online right now. The matrix badge just shows the number of channel members. There's less need for this to be fresh. We tend not to cache things for as long as a day, but I've submitted a PR bumping it to 4 hours at #10778 . That's 480 times longer than we were caching it previously.

Whether or not that gets us unbanned is another issue but caching for longer is something we can do quickly and easily.

Either way, the ability to optionally use the summary endpoint would be an improvement for other reasons.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Jan 1, 2025

Sounds good.

Do you think the logic should be adjusted for now to fetch the summary for matrix.org and for every other server the current logic?

@chris48s
Copy link
Member

chris48s commented Jan 1, 2025

Yes. I think using the summary endpoint for matrix.org only would be a simpler first step and would solve a lot of problems. My hunch is that most of the rooms large enough for the current method to present a performance problem are going to be on matrix.org.

Are you interested in submitting a PR?

Side note: the PR increasing the cache length has been deployed.

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Jan 1, 2025

I agree.

Actually I have already implemented it yesterday. I have not enabled it for the matrix.org by default, I have added an additional parameter "fetchMode" with values "guest" (default) or "summary".

Should I submit it like that or should I change it to make it default for matrix.org and ignore the endpoint for every other homeserver?

@chris48s
Copy link
Member

chris48s commented Jan 1, 2025

I think lets make it the default for matrix.org but not expose fetchMode as a URL param for now

@cyb3rko
Copy link
Contributor Author

cyb3rko commented Jan 1, 2025

Let me quickly explain my thought:

If you're a homeserver owner you may want to be able to reduce load on your server.
Because the summary logic only needs one request while the current logic uses 3 requests per shield call.

My idea would be to enable it by default for matrix.org but still offer a param to overwrite the default for matrix.org or for other homeservers.

But if you say you prefer your way, I will do that.

@chris48s
Copy link
Member

chris48s commented Jan 1, 2025

If we're going to expose it as a param:

  • For matrix.org specifically I'd want to hard-code use of the summary endpoint (i.e: the param should have no effect if server is matrix.org). I don't see a reason we would want the user to be able to specify using the guest account method in the case where we know summary is available.

  • We need to clearly explain all this which requires some work on the docs in https://shields.io/badges/matrix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas
Projects
None yet
Development

No branches or pull requests

2 participants