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

Allow anonymous profile view #1304

Open
wants to merge 10 commits into
base: hometown-dev
Choose a base branch
from

Conversation

ironiridis
Copy link

This PR allows unauthenticated viewers to render a profile if it has not opted out of search engine indexing. Would resolve https://github.com/MSPSocial/projects/issues/4#issue-1581332535, which was unfortunately introduced by mastodon#19803 .

Considerations:

  • This explicitly does not give access to any statuses (ie posts/notes/toots)
  • This behavior depends on the search engine indexing opt-out for lack of a better existing pref.
  • I considered using the followers-require-approval (the so-called "locked") preference, but the search engine preference felt closer to the spirit of the function, as the whole point of disabling unauthenticated API access is to avoid scrapers.

It would probably be better to put this into a new preference, but personally the whole spectrum of privacy preferences in the software should be represented by something more nuanced than a binary. It seems like a fine-grained privacy permissions system would be better here (with each category of data having selectors for audiences, eg: only me, my followers, anybody on my instance, anybody on the fediverse, anybody on the internet) but that's a huge project.

Hence this stopgap. (Cc: @lawremipsum, @t54r4n1)

When viewing a profile anonymously, the request for custom emoji will show a 401 Unauthorized error to the user. Exempt this endpoint from the authentication requirement.
It's helpful to be able to ask the parent class whether we would disallow API access based on the current request (and hence whether we know the requesting user) rather than simply based on configuration. This doesn't appear to regress any existing functionality.
This centralizes the core check for the user's preferences into a single helper method, and gives us one place to change the user preference flag if it turns out user_prefers_noindex isn't appropriate. Also adds a convenience error response specific to the user visibility situation.
app/controllers/api/v1/custom_emojis_controller.rb Outdated Show resolved Hide resolved
Comment on lines +11 to +13
if user_would_block_unauthenticated_api_access?(@account)
user_blocks_unauthenticated_api_access and return
end

Choose a reason for hiding this comment

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

Suggested change
if user_would_block_unauthenticated_api_access?(@account)
user_blocks_unauthenticated_api_access and return
end
if user_would_block_unauthenticated_api_access?(@account)
user_blocks_unauthenticated_api_access and return
end

Since you end up having this statement in a bunch of your controllers, you could move it up to the Api::BaseController and implement it once.

Copy link
Author

Choose a reason for hiding this comment

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

My very basic Ruby grasp is what led me to doing this this way. I needed user_blocks_unauthenticated_api_access to return the JSON object and 401 status, and I wasn't sure how to invoke the render behavior and return from the function. Earlier iterations of this change set had a method that called this, but the calling code would continue and would try to render another result, which resulted in errors in the logs as a response was already being sent. The ... and return pattern was the only way I could think of to solve this, but I'm very open to alternatives (and to learning!)

Choose a reason for hiding this comment

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

I think the and return approach would work still work if you moved this up to a method in the base controller. But maybe not. Controller behavior in Rails is frequently surprising.

app/controllers/api/v1/accounts/lookup_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v1/accounts/statuses_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/base_controller.rb Outdated Show resolved Hide resolved
IanWhitney's review pointed out that it is more consistent and a safer default to include the "only:" directive for all cases where we skip the user authentication requirement. In particular: if a future public method is added to this controller, it should not be implicitly available pre-auth.
It seems I let my inner C programmer get the better of me. Per IanWhitney's review, Ruby's if structures are expressions and return values, which make them more suitable than a ternary expression in terms of readability.
Per IanWhitney's review, the complex logic with the unless clause is pretty clumsy. We can check this condition first and return false immediately, and leave the remaining logic as a simpler statement on its own.
@ironiridis ironiridis requested a review from IanWhitney May 7, 2023 02:59
Comment on lines +11 to +13
if user_would_block_unauthenticated_api_access?(@account)
user_blocks_unauthenticated_api_access and return
end

Choose a reason for hiding this comment

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

I think the and return approach would work still work if you moved this up to a method in the base controller. But maybe not. Controller behavior in Rails is frequently surprising.

if @account.suspended? || disallow_unauthenticated_api_access?
[]
else
cached_account_statuses

Choose a reason for hiding this comment

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

I think this needs one more end. The one on line 29 ends the method. But I don't see an end for the if

Copy link
Author

Choose a reason for hiding this comment

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

Ope, I bet you're right; that's what I get for patching a running EC2 instance and eyeball-copying back to an editor. Will address later today.

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