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

Add session-based auth REST API endpoints for login, logout #11261 #11284

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Aug 5, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

To support arches applications that do not use the Arches login page interface, create REST API endpoints for login and logout. See extended discussion below.

Issues Solved

Closes #11261

Checklist

  • I targeted one of these branches:
    • dev/8.0.x
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Ticket Background

  • Sponsored by: Getty Conservation Institute

Further comments

  • Third-party packages?
    • Django apps with REST APIs often use third-party packages like Django Rest Framework or django-ninja to implement auth.
    • There is value in not reimplementing the auth "wheel" if possible.
    • However, this is most useful if the project has already committed to using those frameworks for their overall REST APIs. AFAICT that decision is not in scope for this ticket.
    • Django Rest Framework encourages you to write a LoginView for your login page anyway and inherit from Django's LoginView, as I've done here, so I'm not convinced the views would look any different if we adopted DRF here: we would just be adding plumbing to avoid boilerplate when subsequent DRF views need authentication classes. But we don't know that we're writing any further DRF views. A project is free to do that if they wish and grab these views as part of that.
  • Session-based or token-based auth?
    • According to DRF:

      Session authentication is appropriate for AJAX clients that are running in the same session context as your website.

    • django-ninja also defaults to session auth
    • We already have an OAuth based token login endpoint, I'd prefer not to unnecessarily add a duplicate.
    • Suggest opting for the simplest implementation until we confront harder use cases (social auth etc).

Testing instructions

Copy link
Contributor

@chrabyrd chrabyrd 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 good, just 2 things to talk about:

  • Is there a chance to DRY up views/auth.py? Or is there enough difference between the interaction that keeping the difference would be prudent?

  • I imagine this is a larger conversation, but is Arches eventually going to use DRF? If so this may be a good opportunity to get our foot in the door.

@jacobtylerwalls
Copy link
Member Author

I imagine this is a larger conversation, but is Arches eventually going to use DRF? If so this may be a good opportunity to get our foot in the door.

Good Q.

  • My inclination is to let arches applications lead the way and demonstrate some benefit before adding a dependency in core. As we discussed re: adopting axios, it doesn't really make sense unless your whole application is using it, and a better case can be made if a project reports good "time to market" when using it.
  • Also, some of our core models might need to change to become stricter about blank=True and default= and that sort of thing given that historically we haven't used full_clean(), whereas DRF serializers will start doing that. It's on my radar to kick off a discussion before we start the Vue cutover.

@jacobtylerwalls
Copy link
Member Author

Or is there enough difference between the interaction that keeping the difference would be prudent?

Yeah, almost every line of that other view deals with redirects and template renders. Even the ratelimit decorator can't be shared, b/c we're doing some gymnastics to double the limit to account for the fact that our auth form requires two clicks to submit.

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@chrabyrd chrabyrd merged commit d22157c into dev/8.0.x Aug 6, 2024
7 checks passed
@chrabyrd chrabyrd deleted the jtw/api-auth branch August 6, 2024 21:09
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.

Implement API-based user authentication
2 participants