florence-social / mastodon-fork

Florence's fork of Mastodon
GNU Affero General Public License v3.0
138 stars 15 forks source link

Nodeinfo 2.0 and 2.1 plus blocks transparency #134

Open rhaamo opened 5 years ago

rhaamo commented 5 years ago

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

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

clarfonthey commented 5 years ago

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 commented 5 years ago

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

clarfonthey commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ghost commented 5 years ago

Ohh that's way better for reporting active users.

rhaamo commented 5 years ago

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

deactivated by default in the settings.yml.

clarfonthey commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

rhaamo commented 5 years ago

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

clarfonthey commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Latest updates:

  1. We're probably going to want to merge in tootsuite/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 commented 5 years ago

@clarfon unfortunately you are better backporting https://github.com/tootsuite/mastodon/pull/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.