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

Fixed members/signin_urls endpoint to take admin api key #21284

Merged
merged 25 commits into from
Oct 16, 2024

Conversation

cathysarisky
Copy link
Contributor

@cathysarisky cathysarisky commented Oct 10, 2024

closes #16748

The members/:member_id/signin_urls endpoint currently only does cookie-based authentication. When #21249 is merged, turning on 2FA is going to break any 3rd party processes that use it (including my social sign-in offering).

This patch gives admin API keys 'read' permission on this endpoint, and enables 3rd party processes to handle user logins the right way, instead of via a staff member's email/password.

Migration included. Feedback appreciated.

I have the wrong name on my migration. I can see it doesn't follow the naming convention, but I'm not sure how the names are generated.

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Oct 10, 2024
Copy link
Contributor

It looks like this PR contains a migration 👀
Here's the checklist for reviewing migrations:

General requirements

  • Satisfies idempotency requirement (both up() and down())
  • Does not reference models
  • Filename is in the correct format (and correctly ordered)
  • Targets the next minor version
  • All code paths have appropriate log messages
  • Uses the correct utils
  • Contains a minimal changeset
  • Does not mix DDL/DML operations
  • Tested in MySQL and SQLite

Schema changes

  • Both schema change and related migration have been implemented
  • For index changes: has been performance tested for large tables
  • For new tables/columns: fields use the appropriate predefined field lengths
  • For new tables/columns: field names follow the appropriate conventions
  • Does not drop a non-alpha table outside of a major version

Data changes

  • Mass updates/inserts are batched appropriately
  • Does not loop over large tables/datasets
  • Defends against missing or invalid data
  • For settings updates: follows the appropriate guidelines

@cathysarisky cathysarisky changed the title [feedback appreciated] Fixed members/signin_urls endpoint to take admin api key Fixed members/signin_urls endpoint to take admin api key Oct 15, 2024
Copy link
Contributor

@9larsons 9larsons left a comment

Choose a reason for hiding this comment

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

Just the one comment. Tested this locally and looks good!

@cathysarisky cathysarisky requested a review from 9larsons October 15, 2024 18:38
@9larsons 9larsons requested a review from mike182uk October 15, 2024 20:01
Copy link
Contributor

@9larsons 9larsons left a comment

Choose a reason for hiding this comment

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

LGTM. I've asked @mike182uk to also take a pass just because of the migration.

…0-01-02-03-add-signin-urls-permissions.js

Co-authored-by: Michael Barrett <[email protected]>
@mike182uk mike182uk merged commit 73a39ea into TryGhost:main Oct 16, 2024
21 checks passed
cmraible pushed a commit to cmraible/Ghost that referenced this pull request Oct 16, 2024
)

closes TryGhost#16748 

The members/:member_id/signin_urls endpoint currently only does
cookie-based authentication. When TryGhost#21249 is merged, turning on 2FA is
going to break any 3rd party processes that use it (including my social
sign-in offering).

This patch gives admin API keys 'read' permission on this endpoint, and
enables 3rd party processes to handle user logins the right way, instead
of via a staff member's email/password.

Migration included.  Feedback appreciated.

I have the wrong name on my migration. I can see it doesn't follow the
naming convention, but I'm not sure how the names are generated.

---------

Co-authored-by: Michael Barrett <[email protected]>
dvdwinden pushed a commit that referenced this pull request Oct 24, 2024
closes #16748 

The members/:member_id/signin_urls endpoint currently only does
cookie-based authentication. When #21249 is merged, turning on 2FA is
going to break any 3rd party processes that use it (including my social
sign-in offering).

This patch gives admin API keys 'read' permission on this endpoint, and
enables 3rd party processes to handle user logins the right way, instead
of via a staff member's email/password.

Migration included.  Feedback appreciated.

I have the wrong name on my migration. I can see it doesn't follow the
naming convention, but I'm not sure how the names are generated.

---------

Co-authored-by: Michael Barrett <[email protected]>
@cathysarisky cathysarisky deleted the fix-signin-by-api branch November 10, 2024 15:41
tilak999 pushed a commit to tilak999/ghost that referenced this pull request Nov 20, 2024
)

closes TryGhost#16748 

The members/:member_id/signin_urls endpoint currently only does
cookie-based authentication. When TryGhost#21249 is merged, turning on 2FA is
going to break any 3rd party processes that use it (including my social
sign-in offering).

This patch gives admin API keys 'read' permission on this endpoint, and
enables 3rd party processes to handle user logins the right way, instead
of via a staff member's email/password.

Migration included.  Feedback appreciated.

I have the wrong name on my migration. I can see it doesn't follow the
naming convention, but I'm not sure how the names are generated.

---------

Co-authored-by: Michael Barrett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration [pull request] Includes migration for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

admin/members/:id/signin_url won't take Owner's staff access token
3 participants