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

Add initial support for creating OAuth2 apps #282

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Conversation

m-novikov
Copy link

@m-novikov m-novikov commented Mar 24, 2022

About this change: What it does, why it matters

Adds ability to configure OAuth2 apps for users.
As we are moving to allow third-party providers to integrate with aiven services it's helpful to have convenient interface to configure oauth2 apps.

@m-novikov m-novikov requested a review from a team March 24, 2022 13:39
@m-novikov m-novikov marked this pull request as ready for review March 24, 2022 13:41
@m-novikov m-novikov requested a review from a team as a code owner March 24, 2022 13:41
Copy link
Contributor

@sjamgade sjamgade left a comment

Choose a reason for hiding this comment

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

LGTM, some questions

body={"redirect_uri": uri},
)

def add_oauth2_client_secret(self, account_id: str, client_id: str) -> Mapping:
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this be get_oauth2.... ?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe create? It does post request and issues new one.

)
def account__oauth2_client__create(self):
"""Create new OAuth2 client."""
oauth2_client = self.client.create_oauth2_client(
Copy link
Contributor

Choose a reason for hiding this comment

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

if something blows up down the line, the client will persist.
Either remove the client then and there or allow it to be removed later ?

Copy link
Author

Choose a reason for hiding this comment

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

Added removal on exception

@m-novikov m-novikov dismissed sjamgade’s stale review March 25, 2022 10:19

addressed comments

@m-novikov m-novikov requested a review from sjamgade March 25, 2022 10:19
@tvainika tvainika changed the title Add intial support for creating OAuth2 apps Add initial support for creating OAuth2 apps Mar 25, 2022
Copy link
Contributor

@sjamgade sjamgade left a comment

Choose a reason for hiding this comment

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

LGTM

@sjamgade
Copy link
Contributor

@tvainika did you want to take a look at this ?
@aiven/aiven-client maybe too ?

Copy link
Contributor

@rominf rominf left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with this topic, but from Aiven Client team member POV, looks fine.

@sjamgade sjamgade merged commit 8e0a0f2 into main Mar 29, 2022
@sjamgade sjamgade deleted the m-novikov-oauth2-cli branch March 29, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants