feat: generate API Credentials in Admin Portal #1833
Conversation
|
A new flag on the Enterprise Customer django admin screen under the subheader “Integration and learning platform settings” called “Allow generation of API credentials”: |
.github/workflows/publish.yml
Outdated
| @@ -26,7 +26,7 @@ jobs: | |||
| - name: Build package | |||
| run: python setup.py sdist bdist_wheel | |||
| - name: Publish to PyPi | |||
| uses: pypa/gh-action-pypi-publish@master | |||
| uses: pypa/gh-action-pypi-publish@release/v1 | |||
There was a problem hiding this comment.
any chance I could get context on this change?
There was a problem hiding this comment.
i'm also curious why this change was made?
enterprise/api/utils.py
Outdated
| Get the enterprise customer name given an user id. | ||
| """ | ||
| try: | ||
| return str(EnterpriseCustomerUser.objects.get(user_id=user_id).enterprise_customer.name) |
There was a problem hiding this comment.
[Question] this has a small bug when the user is assigned to multiple customers but I'm curious if this breaks with the .get
There was a problem hiding this comment.
Is it that one user corresponds to one EnterpriseCustomerUser? If not, how should i determine the value of Application's name for the current user?
There was a problem hiding this comment.
Alex is right here, user_id probably isnt enough here to disambiguate which object you want. I believe there is 1 EnterpriseCustomerUser for every enterprise that an LMS user has access to.
| # Verifies the requesting user is connected to an enterprise that has API credentialing bool set to True | ||
| user = request.user | ||
| if not has_api_credentials_enabled(user.id): | ||
| return Response({'detail': 'Can not access to API credentials.'}, status=status.HTTP_405_METHOD_NOT_ALLOWED) |
There was a problem hiding this comment.
NIT no need for the to ie -
Can not access API credential viewset
Also I don't think 405 is quite the right status code here... 403 might be a little closer https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
| # Verifies the requesting user is connected to an enterprise that has API credentialing bool set to True | ||
| user = request.user | ||
| if not has_api_credentials_enabled(user.id): | ||
| return Response({'detail': 'Can not access to API credentials.'}, status=status.HTTP_405_METHOD_NOT_ALLOWED) |
There was a problem hiding this comment.
NIT same message changes as above
There was a problem hiding this comment.
as well as HTTP status code changes
| # If an application already exists for the user, throw a 409. | ||
| queryset = self.get_queryset() | ||
| if queryset: | ||
| return Response({'detail': 'Application exists.'}, status=status.HTTP_409_CONFLICT) |
There was a problem hiding this comment.
I'm not sure we want to return an error here. If the user hits the endpoint I think we just want to generate a new pair of credentials
There was a problem hiding this comment.
I saw throw a 409 if an application already exists for the user for the CREATE endpoint in the ticket. Should i generate a new pair of credentials?
| feature_role_object, __ = EnterpriseFeatureRole.objects.get_or_create(name=role_name) | ||
| EnterpriseFeatureUserRoleAssignment.objects.get_or_create(user=user, role=feature_role_object) | ||
|
|
||
| application = Application.objects.create( |
There was a problem hiding this comment.
when you create the application object, does it automatically come with a client ID and secret?
| URL: /enterprise/api/v1/enterprise_customer_api_credentials/{user_id} | ||
| """ | ||
| response = {'message': 'partial_update function is not offered in this path.'} | ||
| return Response(response, status=status.HTTP_403_FORBIDDEN) |
There was a problem hiding this comment.
I believe this should be the 405 HTTP status code
| URL: /enterprise/api/v1/enterprise_customer_api_credentials/{user_id} | ||
| """ | ||
| if not has_api_credentials_enabled(request.user.id): | ||
| return Response({'detail': 'Can not access to API credentials.'}, status=status.HTTP_405_METHOD_NOT_ALLOWED) |
| @@ -0,0 +1,327 @@ | |||
| """ | |||
There was a problem hiding this comment.
I see, I think my commit might have been added to your PR, we can fix this
| self.assertEqual(updated_instance.name, self.data['name']) | ||
| self.assertEqual(updated_instance.authorization_grant_type, self.data['authorization_grant_type']) | ||
| self.assertEqual(updated_instance.client_type, self.data['client_type']) | ||
| self.assertEqual(updated_instance.redirect_uris, self.data['redirect_uris']) |
There was a problem hiding this comment.
I believe you need to add a new line
.github/workflows/publish.yml
Outdated
| @@ -26,7 +26,7 @@ jobs: | |||
| - name: Build package | |||
| run: python setup.py sdist bdist_wheel | |||
| - name: Publish to PyPi | |||
| uses: pypa/gh-action-pypi-publish@master | |||
| uses: pypa/gh-action-pypi-publish@release/v1 | |||
There was a problem hiding this comment.
i'm also curious why this change was made?
enterprise/api/utils.py
Outdated
| Get the enterprise customer name given an user id. | ||
| """ | ||
| try: | ||
| return str(EnterpriseCustomerUser.objects.get(user_id=user_id).enterprise_customer.name) |
There was a problem hiding this comment.
Alex is right here, user_id probably isnt enough here to disambiguate which object you want. I believe there is 1 EnterpriseCustomerUser for every enterprise that an LMS user has access to.
yuanyuan-git-tech
force-pushed
the
yuanyuan/ent-7335
branch
from
a4653e8
to
47fd145
Compare
johnnagro
force-pushed
the
yuanyuan/ent-7335
branch
from
47fd145
to
79c2b89
Compare
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @yuanyuan-git-tech! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the |
|
attempting to repackage and rework this #1854 |
|
@yuanyuan-git-tech Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
https://2u-internal.atlassian.net/browse/ENT-7415
Merge checklist:
requirements/*.txtfiles)base.inif needed in production but edx-platform doesn't install ittest-master.inif edx-platform pins it, with a matching versionmake upgrade && make requirementshave been run to regenerate requirementsmake statichas been run to update webpack bundling if any static content was updated./manage.py makemigrationshas been run./manage.py lms makemigrationsin the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgradein edx-platform will look for the latest version in PyPi.