You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/06/23 03:39:52 UTC
[GitHub] [airflow] uranusjr opened a new pull request #16609: API endpoint to create new user
uranusjr opened a new pull request #16609:
URL: https://github.com/apache/airflow/pull/16609
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657065432
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
BadRequest will be fine as I think that it's a user error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657019061
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
+ try:
+ data = user_schema.load(request.json)
+ except ValidationError as e:
+ raise BadRequest(detail=str(e.messages))
+
+ security_manager = current_app.appbuilder.sm
+
+ user = security_manager.find_user(username=data["username"])
+ if user is not None:
+ detail = f"Username `{user.username}` already exists. Please update with PATCH endpoint."
Review comment:
I just realised I didn’t include a PATCH endpoint 😆
```suggestion
detail = f"Username `{user.username}` already exists."
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil closed pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil closed pull request #16609:
URL: https://github.com/apache/airflow/pull/16609
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657050488
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1679,7 +1704,6 @@ components:
name:
type: string
nullable: true
- readOnly: true
Review comment:
So we can assign the user to roles on creation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657212851
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
I kind of feel 400 is not appropriate, since the API client does not know whether a server allows registration or not. I’ll use 403 for now since MDN’s description seems to somehow make sense in this context:
> The access is permanently forbidden and tied to the application logic, such as insufficient rights to a resource.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #16609:
URL: https://github.com/apache/airflow/pull/16609
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657107518
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
Thinking about this again, I wonder whether a 5xx would make sense, since a 4xx indicates the client can fix something to make the request work, but no amount of client-side changes can make this error go away, it is only fixable at the server side (and once it’s resolved, the client can re-send the request unchanged, which is not typical of 4xx either).
Maybe something like 501 Not Implemented?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#issuecomment-866936810
I’ve added PATCH and DELETE, but left out the `AUTH_USER_REGISTRATION` check. The documentation says it toggles *self registration*, but this API is intended to be used by admins to create a user, not for new users to create themselves. So the endpoint shouldn’t be disabled by the configuration IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657107518
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
Thinking about this again, I wonder whether a 5xx would make sense, since a 4xx indicates the client can fix something to make the request work, but no amount of client-side changes can make this error go away, it is only fixable at the server side (and once it’s resolved, the client can re-send the request unchanged, which is not typical of 4xx either).
Maybe something like 501 Not Implemented?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657067893
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
+ try:
+ data = user_schema.load(request.json)
+ except ValidationError as e:
+ raise BadRequest(detail=str(e.messages))
+
+ security_manager = current_app.appbuilder.sm
+
+ user = security_manager.find_user(username=data["username"])
+ if user is not None:
+ detail = f"Username `{user.username}` already exists. Please update with PATCH endpoint."
Review comment:
Should we? Make this PR be the CUD operations to make the UserAPI complete?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#issuecomment-866769320
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657062540
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
Oh, yes we should, will do. (What error should we return, 405 Method Not Allowed?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657047577
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1679,7 +1704,6 @@ components:
name:
type: string
nullable: true
- readOnly: true
Review comment:
Why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#issuecomment-866830106
Indeed, I’m already working on PATCH and DELETE, but want to post this first since it’s more or less standalone, and after this the rest is very straightforward. If it’s desired, I can continue to push to this PR instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#issuecomment-866831647
> Indeed, I’m already working on PATCH and DELETE, but want to post this first since it’s more or less standalone, and after this the rest is very straightforward. If it’s desired, I can continue to push to this PR instead.
Yep. Pushing here is cool in my opinion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657047577
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1679,7 +1704,6 @@ components:
name:
type: string
nullable: true
- readOnly: true
Review comment:
Which this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657060238
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
We can add a check that AUTH_USER_REGISTRATION is True and return early if disabled?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657065958
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
> 405 doesn't quite feel right from an HTTP/REST stand point.
>
> 422 might work (even if it's strictly from WebDAV) https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422
Cool
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657052524
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1679,7 +1704,6 @@ components:
name:
type: string
nullable: true
- readOnly: true
Review comment:
oh well, my bad I confused it for RoleSchema 🤦
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#discussion_r657065029
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -62,3 +64,43 @@ def get_users(limit, order_by='id', offset=None):
users = query.offset(offset).limit(limit).all()
return user_collection_schema.dump(UserCollection(users=users, total_entries=total_entries))
+
+
+@security.requires_access([(permissions.ACTION_CAN_CREATE, permissions.RESOURCE_USER)])
+def post_user():
+ """Create a new user"""
Review comment:
405 doesn't quite feel right from an HTTP/REST stand point.
422 might work (even if it's strictly from WebDAV) https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16609: API endpoint to create new user
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16609:
URL: https://github.com/apache/airflow/pull/16609#issuecomment-872575036
Please rebase on main since pylint is removed now :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org