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-1760] Password authentication for experimental rest API #2730

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@NielsZeilemaker
Contributor

NielsZeilemaker commented Oct 27, 2017

Modified the Password authentication to support HTTP Basic auth

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:
    Implemented HTTP Basic auth to allow Password authentication to be used to secure the experimental api.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    None, the current passwor auth in contrib does not have any tests either

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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 27, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
- Coverage   73.24%   72.59%   -0.66%     
==========================================
  Files         175      154      -21     
  Lines       12331    11866     -465     
==========================================
- Hits         9032     8614     -418     
+ Misses       3299     3252      -47
Impacted Files Coverage Δ
airflow/utils/log/s3_task_handler.py 0% <0%> (-97.15%) ⬇️
airflow/hooks/S3_hook.py 30.95% <0%> (-55.1%) ⬇️
airflow/operators/hive_operator.py 41.02% <0%> (-31.71%) ⬇️
airflow/default_login.py 46.34% <0%> (-12.64%) ⬇️
airflow/utils/log/logging_mixin.py 82.6% <0%> (-12.33%) ⬇️
airflow/utils/log/file_task_handler.py 75.9% <0%> (-11.16%) ⬇️
airflow/utils/email.py 93.12% <0%> (-4.28%) ⬇️
airflow/www/app.py 96.55% <0%> (-2.41%) ⬇️
airflow/hooks/hive_hooks.py 39.52% <0%> (-1.07%) ⬇️
airflow/configuration.py 84.03% <0%> (-0.95%) ⬇️
... and 67 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 f4e3e35...81de93b. Read the comment docs.

codecov-io commented Oct 27, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
- Coverage   73.24%   72.59%   -0.66%     
==========================================
  Files         175      154      -21     
  Lines       12331    11866     -465     
==========================================
- Hits         9032     8614     -418     
+ Misses       3299     3252      -47
Impacted Files Coverage Δ
airflow/utils/log/s3_task_handler.py 0% <0%> (-97.15%) ⬇️
airflow/hooks/S3_hook.py 30.95% <0%> (-55.1%) ⬇️
airflow/operators/hive_operator.py 41.02% <0%> (-31.71%) ⬇️
airflow/default_login.py 46.34% <0%> (-12.64%) ⬇️
airflow/utils/log/logging_mixin.py 82.6% <0%> (-12.33%) ⬇️
airflow/utils/log/file_task_handler.py 75.9% <0%> (-11.16%) ⬇️
airflow/utils/email.py 93.12% <0%> (-4.28%) ⬇️
airflow/www/app.py 96.55% <0%> (-2.41%) ⬇️
airflow/hooks/hive_hooks.py 39.52% <0%> (-1.07%) ⬇️
airflow/configuration.py 84.03% <0%> (-0.95%) ⬇️
... and 67 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 f4e3e35...81de93b. Read the comment docs.

@bolkedebruin

This comment has been minimized.

Show comment
Hide comment
@bolkedebruin

bolkedebruin Oct 30, 2017

Contributor

Tests please and please quash your commits.

Contributor

bolkedebruin commented Oct 30, 2017

Tests please and please quash your commits.

@NielsZeilemaker

This comment has been minimized.

Show comment
Hide comment
@NielsZeilemaker

NielsZeilemaker Nov 10, 2017

Contributor

@bolkedebruin I've added tests and squashed the commits

Contributor

NielsZeilemaker commented Nov 10, 2017

@bolkedebruin I've added tests and squashed the commits

@NielsZeilemaker

This comment has been minimized.

Show comment
Hide comment
@NielsZeilemaker

NielsZeilemaker Jan 10, 2018

Contributor

@bolkedebruin @Fokko would it be possible to merge this pull request?

Contributor

NielsZeilemaker commented Jan 10, 2018

@bolkedebruin @Fokko would it be possible to merge this pull request?

@NielsZeilemaker

This comment has been minimized.

Show comment
Hide comment
@NielsZeilemaker

NielsZeilemaker Jan 16, 2018

Contributor

@Fokko @bolkedebruin any comments? I would really like to get this merged

Contributor

NielsZeilemaker commented Jan 16, 2018

@Fokko @bolkedebruin any comments? I would really like to get this merged

@Fokko

Minor changes, please review, apart from that LGTM. Thanks for providing the tests.

Show outdated Hide outdated airflow/contrib/auth/backends/password_auth.py Outdated
Show outdated Hide outdated airflow/contrib/auth/backends/password_auth.py Outdated
Show outdated Hide outdated airflow/contrib/auth/backends/password_auth.py Outdated
[AIRFLOW-1760] Password auth for experimental API
Modified the Password authentication to support HTTP Basic auth
@Fokko

This comment has been minimized.

Show comment
Hide comment
@Fokko

Fokko Feb 6, 2018

Contributor
Contributor

Fokko commented Feb 6, 2018

@asfgit asfgit closed this in c458a22 Feb 6, 2018

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

[AIRFLOW-1760] Password auth for experimental API
Modified the Password authentication to support
HTTP Basic auth

Closes apache#2730 from NielsZeilemaker/AIRFLOW-1760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment