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

[#432] Implement credentials deletion and generation #446

Merged
merged 45 commits into from
Sep 8, 2020

Conversation

shadcn
Copy link
Contributor

@shadcn shadcn commented Jul 16, 2020

Fixes #432

This PR implements credential generation, status update (revoke/approve) and deletion for key rotation.

[Update: Deleted outdated screenshots]

@googlebot googlebot added the cla: yes Indicates CLA has been signed label Jul 16, 2020
@mxr576
Copy link
Contributor

mxr576 commented Jul 22, 2020

What about regenerating a customer secret on an existing credential?

@cnovak
Copy link
Collaborator

cnovak commented Jul 29, 2020

Approve App

I am not sure, but I believe you may be adding "approve" into this PR which are used for API Providers to approve credentials from developers. This should not be done since the API provider may set up manual approvals on an API Product, and with this change the developer could self approve the app.

Check out what the internal portal is doing. They do not let users delete or approve credentials (they use "key" instead of "credentials" for this entity):

Screen Shot 2020-07-29 at 12 44 26 PM

Generate Credentials

It seems odd to have a tab to create a new credential versus an add button.

@shadcn
Copy link
Contributor Author

shadcn commented Jul 30, 2020

@cnovak OK so we remove approve and keep revoke or do we make revoke the delete action?

@cnovak
Copy link
Collaborator

cnovak commented Jul 31, 2020

What about regenerating a customer secret on an existing credential?

I created #459 to look into this @mxr576

@cnovak
Copy link
Collaborator

cnovak commented Jul 31, 2020

@arshad here are details for you:

Add Credentials

  • The "Add Credentials" should be a button on the credentials listing page.
  • Ideally, a modal would come up to allow user to add details.
  • The add credentials form should not allow user to change the API Products for the app by default. However, add an admin setting to allow API products to be changed per credential.

Approve

  • The approve functionality should be removed.

Revoke

When a credential is revoked, it should follow the integrated portal concept of hiding the revoked credentials when you open the page. There should be a way to see revoked keys:

expand-collapse

  • Currently the system will show a warning for apps that have revoked credentials. This should not be considered a warning state since revoking credentials is part of the maintenance of an app.

Delete

  • The delete should be either removed or made as an optional admin setting to allow deletes on credentials.
  • On an install, the delete functionality should default to not being shown.

@shadcn
Copy link
Contributor Author

shadcn commented Aug 3, 2020

@cnovak Thanks. I'll make the changes.

Question about the following:

The add credentials form should not allow user to change the API Products for the app by default. However, add an admin setting to allow API products to be changed per credential.

OK so when a user adds new credentials, we use the API products from the app? What if we have two credentials with different products on the same app?

On the portal, API products are listed separately from API keys (which makes it look like API products are tied to the app and not keys).

But on both the Drupal portal and Edge UI, each key lists its own products.

Integrated Portal

Test_postal_app___Arshad_Test

Drupal

Hello_app___Apigee

Edge

Apps_-_Apigee

@arlina-espinoza
Copy link
Contributor

@arshad I would use the API products of the first cred as the default, but let me circle back with the Apigee team to check.

@arlina-espinoza
Copy link
Contributor

@arshad We should take the API products from the most recently created credential that is active (not revoked or expired), and have those as the default products for new creds. There is a getIssuedAt() method on the credential entity that would help with that.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Indicates CLA has not been signed and removed cla: yes Indicates CLA has been signed labels Aug 18, 2020
@minnur
Copy link
Contributor

minnur commented Aug 18, 2020

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indicates CLA has been signed and removed cla: no Indicates CLA has not been signed labels Aug 18, 2020
@minnur
Copy link
Contributor

minnur commented Aug 18, 2020

@arshad @arlina-espinoza I also made related fix in the Kickstarter apigee/apigee-devportal-kickstart-drupal#408

@shadcn shadcn marked this pull request as ready for review August 28, 2020 05:44
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Indicates CLA has not been signed and removed cla: yes Indicates CLA has been signed labels Aug 28, 2020
@arlina-espinoza
Copy link
Contributor

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indicates CLA has been signed and removed cla: no Indicates CLA has not been signed labels Aug 28, 2020
Copy link
Contributor

@arlina-espinoza arlina-espinoza left a comment

Choose a reason for hiding this comment

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

Thanks @arshad . I've manually tested this PR and it all looks really good 💯
I only pushed an additional change after the latest conversation with the Apigee team, about the default permissions upon installing: developers should be able to add/revoke keys and edit api products in their own apps, and the same for team admins for their team apps. So the change was to add "edit_api_products developer_app" to the hook install/update.

@arlina-espinoza
Copy link
Contributor

@arshad I've fixed the new functional tests so that they are able to run both with mocks and as integration tests. As part of it, I removed the static app name and consumer key, so that they are able to run (possible concurrently) against the remote API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates CLA has been signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Key Creation/Rotation within one App
6 participants