-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Keycloak: implement client_credentials grant flow #59411
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
Conversation
92e90a8 to
97a9019
Compare
vincbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty cool in general! On top of my comments, could you also update the documentation to mention this new way of authentication?
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py
Outdated
Show resolved
Hide resolved
providers/keycloak/src/airflow/providers/keycloak/auth_manager/services/token.py
Outdated
Show resolved
Hide resolved
providers/keycloak/src/airflow/providers/keycloak/auth_manager/services/token.py
Show resolved
Hide resolved
|
Thanks @vincbeck! I've implemented your suggestions (although mypy is complaining, which I'll fix). When I was doing the changes, I thought of another way to implement client credentials, which might align it more with the specs. We could have just one endpoint, class TokenBody(StrictBaseModel):
"""Token serializer for post bodies."""
grant_type: Literal["password", "client_credentials"] = Field(default="password")
username: str | None = Field(None)
password: str | None = Field(None)
client_id: str | None = Field(None)
client_secret: str | None = Field(None)
@field_validator("username", mode="after")
@classmethod
def validate_username(cls, v, info):
if info.data.get("grant_type") == "password" and v is None:
raise ValueError("username is required for password grant")
return v
@field_validator("password", mode="after")
@classmethod
def validate_password(cls, v, info):
if info.data.get("grant_type") == "password" and v is None:
raise ValueError("password is required for password grant")
return v
@field_validator("client_id", mode="after")
@classmethod
def validate_client_id(cls, v, info):
if info.data.get("grant_type") == "client_credentials" and v is None:
raise ValueError("client_id is required for client_credentials grant")
return v
@field_validator("client_secret", mode="after")
@classmethod
def validate_client_secret(cls, v, info):
if info.data.get("grant_type") == "client_credentials" and v is None:
raise ValueError("client_secret is required for client_credentials grant")
return v |
|
I like that! I agree, from a user standpoint, that will be easier to use one endpoint instead of multiple. Regarding |
ae42240 to
20339d4
Compare
|
Sorry for the delay @vincbeck. Keeping just one endpoint and using pydantic unions makes everything very clean. It has been a bit difficult to make it work since we should keep Checks are green now. |
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/token.py
Outdated
Show resolved
Hide resolved
|
Thanks for pointing that out, @dabla! It indeed makes the code even cleaner. I had a ValueError there just for mypy. Pydantic already checks that the I've added the |
e1cb208 to
36d57e4
Compare
Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
36d57e4 to
b58a613
Compare
|
Can you please resolve the conflicts? |
vincbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, only some nits
providers/keycloak/src/airflow/providers/keycloak/auth_manager/datamodels/token.py
Outdated
Show resolved
Hide resolved
providers/keycloak/src/airflow/providers/keycloak/auth_manager/datamodels/token.py
Outdated
Show resolved
Hide resolved
|
I love that feature! |
|
Thanks @vincbeck ! |
|
Nice! |
* kc: implement client_credentials grant * Improve error log Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * change client credentials service func name Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * update client credentials service usage * refresh token can be None * add docs for client credentials * implement pydantic union * add tests for the new token route * add docs for new token endpoint * fix mypy error * refactor token creation logic to use methods in data models * lowercase Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * lowercase * remove unused import --------- Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
* kc: implement client_credentials grant * Improve error log Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * change client credentials service func name Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * update client credentials service usage * refresh token can be None * add docs for client credentials * implement pydantic union * add tests for the new token route * add docs for new token endpoint * fix mypy error * refactor token creation logic to use methods in data models * lowercase Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * lowercase * remove unused import --------- Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
closes: #59286
This PR implements the client_credentials grant flow to enable other services use of the Airflow API.
It modifies
POST /auth/tokento add agrant_typeoption (by defaultpasswordto not break backwards compatibility).When using
grant_type=client_credentialsit requires passing theclient_idandclient_secretin order to obtain the token for the associated service account.When using
grant_type=passwordit requires passing theusernameandpassword, as it was until now.The
clientused must exist in the same realm and instance configured for the auth manager. The service account must have the appropriate permissions / roles for the resources it needs access.I've tested obtaining the token and using it with
airflow.sdk.api.client.Clientand it works.Some considerations:
refresh_usermethod.airflowctl, but it should work by setting the env variableAIRFLOW_CLI_TOKENwith the obtained token.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.