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

Adds 2023-12-19-ADMIN_AUTH.md first pass #4

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

Conversation

hhff
Copy link
Member

@hhff hhff commented Dec 19, 2023

Copy link

@wkentdag wkentdag left a comment

Choose a reason for hiding this comment

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

Fix typo, add a detail to google SMTP section

rfcs/2023-12-19-ADMIN_AUTH.md Outdated Show resolved Hide resolved
rfcs/2023-12-19-ADMIN_AUTH.md Outdated Show resolved Hide resolved
hhff and others added 4 commits December 19, 2023 19:41
Co-authored-by: wkd <3254957+wkentdag@users.noreply.github.com>
Co-authored-by: wkd <3254957+wkentdag@users.noreply.github.com>
Copy link

@wkentdag wkentdag left a comment

Choose a reason for hiding this comment

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

This looks great! Left a couple comments, the only one that I think is in-scope for this RFC is the question about handling instance ownership updates. With a bit more detail in that section I think this is good to go!

Comment on lines +61 to +63
**Note:** Rather than providing an open SMTP server for the instance to authenticate and
connect to, we'll offer an API that triggers a pre-formatted authentication email (rather
than an open SMTP server which could be more widely abused).

Choose a reason for hiding this comment

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

Would this flow be any different in the future, when an artist could supply their own SMTP creds? Seems like a safe assumption to me...I guess somebody might want to override the rate limit in this case, but can't imagine why 5/hr wouldn't be sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, if an artist supplies their own SMTP creds, we'd essentially "undock" the instance, and their auth email flow would just go direct to their instance.

& Ya, a rate limiter wouldn't be quite as important in that internal case because it's all internal requests (not a public API)

we'll ask the Dome.fm instance if it's registered itself (we'll likely check the PostgresQL
database for a `MothershipAPIKey`, ie `const hasMothershipAPIKey = !!(await prisma.mothershipAPIKey.findMany())[0]`)

2. If `false`, we'll `POST https://developers.dome.fm/instance-api/[...domeInstanceVersion]/instances` with a body

Choose a reason for hiding this comment

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

We'll probably want to version this API since instances could be stale.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya that's why [...domeInstanceVersion] is in the URL path! The instance must pass it's own version so that we can ensure we don't break compat :)

Choose a reason for hiding this comment

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

Lol sorry, I read right past that 🤦

Comment on lines +140 to +142
In order to ensure we only send magic link emails to the appropriate instance admins (and not
a malicious user), we'll check that the `email` field given is actually a known admin by the
Mothership API.

Choose a reason for hiding this comment

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

we'll check that the email field given is actually a known admin by the
Mothership API.

Could you add a bit more detail on how we'd perform this check? I assume that the Mothership would store the owner included with requests to the /instances endpoint. In order to support ownership changes / multiple admins on a single instance, we'll also need to notify the Mothership when there are changes to the instance "team".

Copy link
Member Author

@hhff hhff Dec 29, 2023

Choose a reason for hiding this comment

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

Could you add a bit more detail on how we'd perform this check? I assume that the Mothership would store the owner included with requests to the /instances endpoint.

Correct. The Instance - has_many -> Administrator (role: owner)

In order to support ownership changes / multiple admins on a single instance, we'll also need to notify the Mothership when there are changes to the instance "team"

Ya, exactly. Annoying I know but ultimately good for the ecosystem's security.

The instances will need to sync their state against the mothership API periodically. It could be done on a per-request basis, but honestly I'm kinda thinking a better way to do that would be to periodically pg_dump the entire instance DB and send it to the mothership.

Why? That's kinda heavy handed...

Well because if we want to support a player app & centralized discovery (ie, dome.fm/search-artists) we'd need to essentially copy the entire DB of the instance anyhow. & If a fan logs into a player app, we'd need to know everything they've bought from various Dome instances... which requires us to aggregate everything into one big collection to power that experience.

(Of course, this periodic sync could be "turned off", ie, you could "undock" your instance completely, but you'd be excluded from centralized discovery & player app)

cc @wkentdag

Choose a reason for hiding this comment

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

Cool. That makes sense to me. Certainly a reasonable place to start anyway, we could make it more surgical down the line if necessary.

Copy link

@wkentdag wkentdag left a comment

Choose a reason for hiding this comment

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

thanks for the followup @hhff 🚀

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