Skip to content
This repository has been archived by the owner on Aug 2, 2020. It is now read-only.

Nodeinfo 2.0 and 2.1 plus blocks transparency #134

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

Conversation

rhaamo
Copy link

@rhaamo rhaamo commented Jul 12, 2019

This is a respin of #38 but with the difference:

  • nodeinfo 2.1 have been added again
  • fixed metaData -> metadata
  • better count for active users from gled-rs fork

This nodeinfo is valid per fedinet nodeinfo validator: https://fediverse.network/tools/nodeinfo/dashie.masto.dev.otter.sh

My ruby/rails is very rusty, please tell me if any issue.

Also might need a more real world test with more than three blocks to see if any issue.

Closes #21

gled-rs added a commit to gled-rs/mastodo that referenced this pull request Jul 12, 2019
@clarfonthey
Copy link
Member

So, we did discuss that if we do include blocks, that this be an opt-in feature, as some instances prefer to not publicise blocks. Other than that, it should be good, I think.

@mal0ki
Copy link
Member

mal0ki commented Jul 12, 2019

Let's also make sure we can test this somewhere before release?

@clarfonthey
Copy link
Member

The link given shows a test on an instance so I assume that qualifies. :p

Although it may be good to include a test in the code itself-- we could add this later as an integration test.

@rhaamo
Copy link
Author

rhaamo commented Jul 12, 2019

I think I can manage to add a flag in .env to activate it but I'm not sure at all I can do it with a nice checkbox somewhere in admin as I don't know the mastodon internals at all.

If it's ok I can do it, the blocklist will be disabled by default.

NODEINFO_BLOCKS_TRANSPARENCY would be a good choice ?

There is some tests in the initial PR but as said, I don't know the mastodon codebase so if someone can make something better, feel free to do that.

@clarfonthey
Copy link
Member

#32 is a great example of adding a config option to the admin UI, and it's not actually too much more effort. That said, this already is a helpful change and we can add that in later if you're more comfortable making it configured with an environment variable for now!

I'd make the env variable just NODEINFO_SHOW_BLOCKS because transparency is a bit vague to me.

@rhaamo
Copy link
Author

rhaamo commented Jul 12, 2019

would it be a good place right after the profile discovery thingy in the /edit admin page ?

@ghost
Copy link

ghost commented Jul 12, 2019

Ohh that's way better for reporting active users.

@rhaamo
Copy link
Author

rhaamo commented Jul 12, 2019

Capture d’écran 2019-07-13 à 01 26 55

deactivated by default in the settings.yml.

@clarfonthey
Copy link
Member

I would say "nodeinfo API" because people may not know what nodeinfo is, and API is a bit more recognisable of a term. Maybe in the longer description also it might be good to call it "public API" to make it clear that anyone can see it.

Also if you want to add a translation for the French version in this PR too you can add it to the .fr version of the file with the strings.

Thank you for the work on this!

@clarfonthey
Copy link
Member

clarfonthey commented Jul 12, 2019

Also for the test error, there is a bundle command you can run. It should be listed in the other PR, although I'm on my phone and don't have time to find it.

EDIT: It's bundle exec i18n-tasks normalize.

@rhaamo
Copy link
Author

rhaamo commented Jul 12, 2019

I've found it's bundler exec i18n-tasks normalize but I have an issue with fuubar which 2.4.0 have been removed, rendering bundler unable to do anything:

$ bundle install
Fetching gem metadata from https://rubygems.org/............
Your bundle is locked to fuubar (2.4.0), but that version could not be found in any of the sources listed in your Gemfile. If you haven't changed sources, that means the author of fuubar (2.4.0) has removed it. You'll need to update your bundle to a version other than
fuubar (2.4.0) that hasn't been removed in order to install.

is there some workaround for this ?

@rhaamo
Copy link
Author

rhaamo commented Jul 13, 2019

nevermind I've edited temporarily the .lock and run the command.

@clarfonthey clarfonthey added B-fediverse (Benefits) fediverse and non-mastodon instances C-api API (component) K-fediverse-extensions (Kind) is non-standard fediverse features labels Jul 13, 2019
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

LGTM overall. I'm not sure about the option name/label, as I'm considering displaying a list of blocked instances on the about page in Mastodon or glitch-soc (with settings such as: don't display it, display it only to logged-in users, display it to everyone)

app/controllers/well_known/node_info_controller.rb Outdated Show resolved Hide resolved
app/serializers/node_info_serializer.rb Show resolved Hide resolved
@rhaamo
Copy link
Author

rhaamo commented Jul 18, 2019

rebased the PR. can I have some ideas on when this is going to be merged ?

@clarfonthey
Copy link
Member

The current plan is to merge it once 0.1.1 is released so that we can add it as part of 0.2.

@clarfonthey
Copy link
Member

So, now that 0.1.1 is out, I'm getting back to looking at this.

I think that longer term we're going to want to have a test that verifies that blocks don't get shown when the setting is disabled. For this PR, it should be fine without one, but longer term we're going to want a regression test for revealing potentially sensitive data. I'll open an issue depending on what happens for that.

From a first glance at the code, the active user count is already stored in the database, so, this just reads that value, I think? Would you be willing to clarify on that?

Finally, once #156 merges we're going to want to replace the software displayed to say florence-mastodon and the version to be Florence::Version.to_s. Will have to double-check in mattermost if we want to settle with this, but I think this will longer term be okay, even if we end up usually using the shortening of flomo in most places.

If you're not up to making these changes, let me know and I can help out. Thanks again for your work on this! <3

@rhaamo
Copy link
Author

rhaamo commented Jul 31, 2019

There shouldn't be issues with the true/false display of the blocks, if there is one it would be in the mastodon settings core and there might be more other weird things happenings than just displaying blocks where it shouldn't.

I think testing it would "just" be adding domain blocks and then checking nodeinfo with the others actual tests, not sure I really want to dig in this but if domains blocks are already tested, some code can be reused.

For the active user count (for "half year"), from what @gled-rs told me, reading it could cause timeouts on big databases, hence the new method who will use redis to store it instead of re-counting it every time.

Ok for the name but tell me when you want it changed, same for the version class.
I'm not rebasing until it's ready to be merged.

Thoses changes can also be done later it's a matter of changing two lines.

@clarfonthey
Copy link
Member

Ah, the new database access is just caching it in Redis, so, makes sense! I may look a bit more closely to see if we can cache the values as background jobs somehow so it's not always on nodeinfo accesses.

In terms of the test, it's mostly a regression test like I said-- I'm confident the existing implementation works as expected but I just want to make sure that nothing in the future gets changed in a way that makes the test fail.

@clarfonthey
Copy link
Member

Also yeah, I don't expect you to rebase until the change is ready. Just wanted to keep you in the loop. Will let you know when we're ready. :)

@rhaamo
Copy link
Author

rhaamo commented Jul 31, 2019

If I understand correctly, since it's in app/models/user.rb:

  def prepare_returning_user!
    ActivityTracker.record('activity:logins', id)
    ActivityTracker.record_month('activity:logins_month', id)
    regenerate_feed! if needs_feed_update?
  end

It should be called on user activity then ? @ThibG might be able to confirm.

@ClearlyClaire
Copy link
Contributor

This indeed records user activity in a redis HyperLogLog structure each time there is user activity. This is fast. The reading part should be somewhat reasonably fast, while still slower. Which isn't much of an issue, because it is cached using Rail's cache. Performances should be fine.

@clarfonthey
Copy link
Member

Latest updates:

  1. We're probably going to want to merge in Add public blocks to /about/blocks mastodon/mastodon#11298 before we merge this so we can use the same settings for both.
  2. Florence::Version has been added finally, so, we'll want to use that for publishing the version.

If you're interested in still working on this, let us know, otherwise I can take this on!

@rhaamo
Copy link
Author

rhaamo commented Sep 2, 2019

@clarfon unfortunately you are better backporting mastodon/mastodon#11628 which include more fixes.
So if you backport 11298, it seems to use the setting from the public blocks display, however backporting it removes nodeinfo 2.1 (it keeps 2.0), maybe that's not an issue for you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B-fediverse (Benefits) fediverse and non-mastodon instances C-api API (component) K-fediverse-extensions (Kind) is non-standard fediverse features
Development

Successfully merging this pull request may close these issues.

nodeinfo support
5 participants