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

Implement RFC 8414 for OAuth 2.0 server metadata #29191

Merged
merged 7 commits into from
May 6, 2024

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Feb 13, 2024

This implements #24099 by doing it directly in Mastodon's codebase, rather than trying to upstream it (upstream were offered and ignored the offer from me to implement it).

I've tried to emulate the code we have elsewhere, and used the Doorkeeper OpenID Connect gem as a reference for some things that are common between RFC 8414 and OpenID Connect specifications.

We could additionally support ui_locales_supported, if we have BCP 47 values available for the languages the UI of Mastodon supports (note: we currently don't support localisation of OAuth 2.0 Application names or localised TOS/privacy URLs, as described in RFC 7591 Section 2.2), I'm not sure what the values returned from LanguagesHelper::SUPPORTED_LOCALES are and if they are BCP 47 compliant.

Example Response:

{
  "issuer": "http://mastodon.lan:3000/",
  "service_documentation": "https://docs.joinmastodon.org/",
  "authorization_endpoint": "http://mastodon.lan:3000/oauth/authorize",
  "token_endpoint": "http://mastodon.lan:3000/oauth/token",
  "app_registration_endpoint": "http://mastodon.lan:3000/api/v1/apps",
  "revocation_endpoint": "http://mastodon.lan:3000/oauth/revoke",
  "scopes_supported": [
    "read",
    "write",
    "write:accounts",
    "write:blocks",
    "write:bookmarks",
    "write:conversations",
    "write:favourites",
    "write:filters",
    "write:follows",
    "write:lists",
    "write:media",
    "write:mutes",
    "write:notifications",
    "write:reports",
    "write:statuses",
    "read:accounts",
    "read:blocks",
    "read:bookmarks",
    "read:favourites",
    "read:filters",
    "read:follows",
    "read:lists",
    "read:mutes",
    "read:notifications",
    "read:search",
    "read:statuses",
    "follow",
    "push",
    "admin:read",
    "admin:read:accounts",
    "admin:read:reports",
    "admin:read:domain_allows",
    "admin:read:domain_blocks",
    "admin:read:ip_blocks",
    "admin:read:email_domain_blocks",
    "admin:read:canonical_email_blocks",
    "admin:write",
    "admin:write:accounts",
    "admin:write:reports",
    "admin:write:domain_allows",
    "admin:write:domain_blocks",
    "admin:write:ip_blocks",
    "admin:write:email_domain_blocks",
    "admin:write:canonical_email_blocks",
    "crypto"
  ],
  "response_types_supported": [
    "code"
  ],
  "response_modes_supported": [
    "query",
    "fragment",
    "form_post"
  ],
  "grant_types_supported": [
    "authorization_code",
    "password",
    "client_credentials"
  ],
  "token_endpoint_auth_methods_supported": [
    "client_secret_basic",
    "client_secret_post"
  ]
}

We use the non-standard app_registration_endpoint as that endpoint isn't technically compliant with OAuth 2.0 Dynamic Client Registration. It's similar but not the same.

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 97.29730% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 85.02%. Comparing base (86500e3) to head (56f313e).
Report is 665 commits behind head on main.

❗ Current head 56f313e differs from pull request most recent head a26efe8. Consider uploading reports for the commit a26efe8 to get more accurate results

Files Patch % Lines
app/presenters/oauth_metadata_presenter.rb 96.42% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #29191   +/-   ##
=======================================
  Coverage   85.01%   85.02%           
=======================================
  Files        1059     1063    +4     
  Lines       28277    28349   +72     
  Branches     4538     4543    +5     
=======================================
+ Hits        24040    24103   +63     
- Misses       3074     3078    +4     
- Partials     1163     1168    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThisIsMissEm ThisIsMissEm marked this pull request as ready for review February 14, 2024 18:26
@ThisIsMissEm
Copy link
Contributor Author

I think this is now good for review, and hopefully merge in to main for 4.3 (with possibly backporting)

@renchap
Copy link
Sponsor Member

renchap commented Feb 14, 2024

I think this is now good for review, and hopefully merge in to main for 4.3 (with possibly backporting)

We generally do not backport features, except if they are motivated by security.

@ThisIsMissEm
Copy link
Contributor Author

I think this is now good for review, and hopefully merge in to main for 4.3 (with possibly backporting)

We generally do not backport features, except if they are motivated by security.

@renchap I think I can make an argument for that: by advertising scopes available, we encourage third-party developers to negotiate the scopes required, reducing access to data.

e.g., currently if I want to access the /api/v1/admin/measures endpoint to read relationship stats between that server and another server, on 4.2 I have to ask for admin:read scope, which gives a tonne of access. But if in the future we introduce a narrower scope, e.g., admin:read:measures or admin:read:measures:relationships then my third-party app could use that narrower permission, and not need admin:read

@renchap renchap added this to the 4.3.0 milestone Mar 25, 2024
@ThisIsMissEm ThisIsMissEm changed the title First pass at implementing RFC 8414 for OAuth 2.0 server metadata Implement RFC 8414 for OAuth 2.0 server metadata Mar 29, 2024
@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Apr 24, 2024

I'm not familiar with RFC 8414, but I'll be looking into it.

One question I have is how it interacts with LOCAL_DOMAIN VS WEB_DOMAIN and possibly multi-tenancy: if you have https://ap.example.com/users/bob serving @bob@example.com, do you want the OAuth 2.0 Authorization Server Metadata available at https://example.com/.well-known/oauth-authorization-server? Wouldn't that interfere with potential other services at example.com?

@ThisIsMissEm
Copy link
Contributor Author

ThisIsMissEm commented Apr 24, 2024

I'm not familiar with RFC 8414, but I'll be looking into it.

One question I have is how it interacts with LOCAL_DOMAIN VS WEB_DOMAIN and possibly multi-tenancy: if you have https://ap.example.com/users/bob serving @bob@example.com, do you want the OAuth 2.0 Authorization Server Metadata available at https://example.com/.well-known/oauth-authorization-server? Wouldn't that interfere with potential other services at example.com?

I believe you'd want it at the web domain. When we do OAuth Logins, we pass the server's web domain as where to authenticate, so the URLs in here should all be the WEB_DOMAIN, not local domain.

All the URLs are generated by Doorkeeper, so that should all be using the correct domain.

That is to say, in your example, to login with OAuth, a user would need to enter https://ap.example.com and we'd then "discover" https://ap.example.com/.well-known/oauth-authorization-server; If you wanted to support a webfinger based oauth flow, then you'd need something in that Actor document or Webfinger response to indicate where the users' oauth authorization server is.

OIDC does have support for webfinger based discovery, but we're using OAuth 2, so we don't yet have a mechanism in webfinger for this.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue May 6, 2024
Merged via the queue into mastodon:main with commit 116f01e May 6, 2024
29 checks passed
@ThisIsMissEm ThisIsMissEm deleted the feat/rfc-8414-support branch May 23, 2024 11:48
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.

None yet

5 participants