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

[AIRFLOW-1836] airflow uses OAuth Provider keycloak #2799

Closed
wants to merge 2 commits into from
Closed

[AIRFLOW-1836] airflow uses OAuth Provider keycloak #2799

wants to merge 2 commits into from

Conversation

fisher-monkey
Copy link

@fisher-monkey fisher-monkey commented Nov 20, 2017

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

@codecov-io
Copy link

codecov-io commented Nov 20, 2017

Codecov Report

Merging #2799 into master will decrease coverage by 3.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2799      +/-   ##
==========================================
- Coverage   76.67%   73.52%   -3.15%     
==========================================
  Files         199      158      -41     
  Lines       16186    11958    -4228     
==========================================
- Hits        12410     8792    -3618     
+ Misses       3776     3166     -610
Impacted Files Coverage Δ
airflow/operators/email_operator.py 0% <0%> (-100%) ⬇️
airflow/hooks/pig_hook.py 0% <0%> (-100%) ⬇️
airflow/operators/slack_operator.py 0% <0%> (-97.37%) ⬇️
airflow/operators/s3_file_transform_operator.py 0% <0%> (-96.23%) ⬇️
airflow/operators/redshift_to_s3_operator.py 0% <0%> (-95.46%) ⬇️
airflow/hooks/mssql_hook.py 6.66% <0%> (-66.67%) ⬇️
airflow/hooks/hdfs_hook.py 32.5% <0%> (-60%) ⬇️
airflow/operators/hive_operator.py 41.02% <0%> (-45.52%) ⬇️
airflow/hooks/S3_hook.py 56.97% <0%> (-37.35%) ⬇️
airflow/hooks/hive_hooks.py 39.52% <0%> (-33.9%) ⬇️
... and 196 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e703d6b...4edc73c. Read the comment docs.

@Fokko
Copy link
Contributor

Fokko commented Nov 21, 2017

Hi @fisher-monkey, thanks for contributing. Could you open a Jira ticket and add a description about the implementation that you've did?


def get_ghe_user_profile_info(self, ghe_token):
#resp = self.ghe_oauth.get(self.ghe_api_route('/userinfo'),token=(ghe_token, ''))
#resp=self.ghe_oauth.get('http://10.110.13.147:9090/auth/realms/brent/protocol/openid-connect/userinfo',token=(ghe_token,''))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comments

Copy link
Author

Choose a reason for hiding this comment

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

I have create a Jira ticket and simply describe the implementation, and this is the link: https://issues.apache.org/jira/browse/AIRFLOW-1836

the comments have been removed.


username, email = self.get_ghe_user_profile_info(ghe_token)
except AuthenticationError:
_log.exception('')
Copy link
Contributor

Choose a reason for hiding this comment

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

An empty log line

from airflow.configuration import AirflowConfigException

import os
os.environ['OAUTHLIB_INSECURE_TRANSPORT'] = '1'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be disabled by default

@ron819
Copy link
Contributor

ron819 commented Oct 3, 2018

@fisher-monkey @Fokko is there any progress with this?

@c4milo
Copy link
Contributor

c4milo commented Oct 4, 2018

Would it be possible to make this more generic and support any OpenIDConnect/OAuth2 identity provider? being KeyCloak one of them.

@Fokko
Copy link
Contributor

Fokko commented Oct 12, 2018

Very good suggestion @c4milo

@ashb ashb changed the title airflow uses OAuth Provider keycloak [AIRFLOW-1836] airflow uses OAuth Provider keycloak Nov 14, 2018
@ron819
Copy link
Contributor

ron819 commented Nov 14, 2018

@Fokko Does the suggestion to make it generic prevents from this to be merged or it's a feature request for future development? We do have other cases where non-generic PRs have been submitted. Example: #3941

@Fokko
Copy link
Contributor

Fokko commented Nov 18, 2018

I think it is for #3941 hard to implement it generic, i.e. have some logic to test the connection of an arbitrary connection. Implementing OAuth makes perfect sense. What do you suggest @ron819 ?

@stale
Copy link

stale bot commented Jan 2, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 2, 2019
@stale stale bot closed this Jan 10, 2019
@weldpua2008
Copy link
Contributor

@Fokko / @fisher-monkey Could you please inform if keycloak is supported in Airflow?
We started to use keycloak and I need to configure it.

@mik-laj
Copy link
Member

mik-laj commented Aug 12, 2020

@weldpua2008 You can configure any OAuth2 provider, but I am working on a Keycloak-proxy integration as it is much more secure. It's not public yet, but I have plans to share it with the community.

@farisabdulraheem
Copy link

How can i implement keycloak auth with airflow? @fisher-monkey @Fokko is there a simple way for that, any sample code available ?

@mik-laj
Copy link
Member

mik-laj commented Oct 20, 2020

this PR refers to the old and unsupported user interface. We support RBAC (Flask AppBuilser based) only now.

@farisabdulraheem
Copy link

@mik-laj so how can i use it with keycloak using rbac

@weldpua2008
Copy link
Contributor

Any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants