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

Refactor Authenticators to make use of inheritance #856

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Jan 23, 2025

Replaces the use of the Mode enum with inheritance from two abstract base classes that provide different authentication mechanisms. Added some type hints.

Replaced "password" in AboutAuthenticationProvider with "internal", which is then a matching pair with "external".

Checklist

  • Add a Changelog entry
  • Add the ticket number which this PR closes to the comment section

import pamela

try:
pamela.authenticate(username, password, service=self.service)
return UserSessionState(username, {})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO this is clearer than try/except/else: if pamela.authenticate doesn't throw an exception, we go from line 100 to line 101, rather than to line 110, and the happy path all lives together

@DiamondJoseph DiamondJoseph force-pushed the authenticator-refactor branch from 96a1929 to c577f06 Compare January 23, 2025 12:54
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

I like this. One depedency-management issue noted in line.

from urllib.parse import parse_qs, urlparse

import httpx
import platformdirs

from tiled.server.schemas import AboutAuthenticationProvider
Copy link
Member

Choose a reason for hiding this comment

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

This pulls in server-side deps. This keeps coming up; maybe we should move these schemas into tiled.schemas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, the schema is being published by the server on some endpoint, and the client is fetching this endpoint to build its understanding of the server? I'd like to when that is done to have a typed object rather than a plain dict. I'll see what can be done.

@DiamondJoseph DiamondJoseph force-pushed the authenticator-refactor branch from d990f1b to ca28584 Compare January 24, 2025 10:24
@DiamondJoseph
Copy link
Contributor Author

I may rewrite history a little if this passes and move the schema stuff to a seperate PR.

@DiamondJoseph DiamondJoseph force-pushed the authenticator-refactor branch from ca28584 to 5d96a83 Compare January 24, 2025 13:23
@DiamondJoseph
Copy link
Contributor Author

This will be rebased after #862 goes in

@danielballan
Copy link
Member

Ready for rebase. Please add a CHANGELOG entry also.

@DiamondJoseph DiamondJoseph force-pushed the authenticator-refactor branch 3 times, most recently from af16bcc to 8816cc7 Compare January 27, 2025 11:18
@DiamondJoseph DiamondJoseph force-pushed the authenticator-refactor branch from 8816cc7 to d1f00f9 Compare January 27, 2025 11:31
@danielballan danielballan merged commit 444515c into main Jan 27, 2025
9 checks passed
@DiamondJoseph DiamondJoseph mentioned this pull request Jan 28, 2025
2 tasks
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