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.

Sign up for GitHub

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

Jump to bottom

custom AuthConnector for OAuth2 #28004

Open

gusega wants to merge 10 commits into backstage:master from gusega:custom_auth_connector

+431 −115

Contributor

gusega commented Dec 5, 2024 •

edited

Loading

Hey, I just made a Pull Request!

Provide custom authConnector to Oauth2

✔️ Checklist

A changeset describing the change and affected packages. (more info)
Added or updated documentation
Tests for new functionality and regression tests for bug fixes
Screenshots attached (for UI changes)
All your commits have a Signed-off-by line in the message. (more info)


          custom AuthConnector for OAuth2

1e0230e

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>

gusega requested review from a team as code owners

December 5, 2024 18:17

gusega requested review from freben and vinzscam

December 5, 2024 18:17

github-actions bot added the auth label

Contributor

backstage-goalie bot commented Dec 5, 2024 •

edited

Loading

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/core-app-api	packages/core-app-api	minor	`v1.15.4`

gusega mentioned this pull request

🚀 Feature: Use custom implementation of AuthConnector with backstage RefreshingAuthSessionManager #27913

Open

2 tasks

Contributor Author

gusega commented Dec 5, 2024

this is for #27913

Gasan Guseinov added 2 commits

December 9, 2024 15:33


          update api specification

976b4fe

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>


          update api specification

fe01586

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>

Rugvip requested changes

View reviewed changes

Member

Rugvip left a comment

Thank you! 👍

The new API surface looks alright and we can go ahead with this. Just a bunch of nits now that these things around about to become public API, things we don't care about as much for internal APIs.

packages/core-app-api/report.api.md Outdated

Comment on lines 576 to 578

    
                authConnectorFactory?: (

                  opts: OAuth2CreateOptions,

                ) => AuthConnector<OAuth2Session>;

Member

Rugvip Dec 17, 2024

Could this just be authConnector?: AuthConnector<OAuth2Session>? The opts type isn't really correct since these fields have been defaulted, but we wouldn't want to add another type. Overall it seems a bit unnecessary to wrap up like this

Contributor Author

gusega Jan 7, 2025

ok, got it. The downside of providing ready AuthConnector is that user must take care of providing same scopeTransform option to both OAuth2#create(...) and in the passed connector.

packages/core-app-api/report.api.md Outdated

    
              export type LoginPopupOptions = {

                url: string;

                name: string;

                origin: string;

Member

Rugvip Dec 17, 2024

Could you move this into showLoginPopup to be derived from the url instead?

packages/core-app-api/report.api.md Outdated

    
            @@ -478,6 +491,15 @@ export class LocalStorageFeatureFlags implements FeatureFlagsApi {
          
                save(options: FeatureFlagsSaveOptions): void;

              }

              // @public

              export type LoginPopupOptions = {

Member

Rugvip Dec 17, 2024

Rename this one to ShowLoginPopupOptions, or matching w/e name the function ends up with

packages/core-app-api/report.api.md Outdated

    
            @@ -637,6 +662,9 @@ export class SamlAuth
          
                signOut(): Promise<void>;

              }

              // @public

              export function showLoginPopup(options: LoginPopupOptions): Promise<any>;

Member

Rugvip Dec 17, 2024

Thinking openLoginPopup might be a better name for this one?

It needs a more explicit return type as well, can't have any. Needs to either be narrowed down or switched to unknown

packages/core-app-api/report.api.md Outdated

    
            @@ -353,6 +360,12 @@ export function createFetchApi(options: {
          
                middleware?: FetchMiddleware | FetchMiddleware[] | undefined;

              }): FetchApi;

              // @public (undocumented)

              export type CreateSessionOptions = {

Member

Rugvip Dec 17, 2024

Rename this ot AuthConnectorCreateSessionOptions

packages/core-app-api/report.api.md Outdated

    
              // @public

              export type AuthConnector<AuthSession> = {

                createSession(options: CreateSessionOptions): Promise<AuthSession>;

                refreshSession(scopes?: Set<string>): Promise<AuthSession>;

Member

Rugvip Dec 17, 2024

Could you wrap this one up in options as well? So options?: AuthConnectorRefreshSessionOptions with scopes inside

.changeset/brave-pandas-beam.md Outdated

		@@ -0,0 +1,5 @@
		---
		'@backstage/core-app-api': patch

Member

Rugvip Dec 17, 2024

Gonna be a minor for this one given the new API

.changeset/brave-pandas-beam.md Outdated

+              '@backstage/core-app-api': patch
+              ---
+              custom AuthConnector for OAuth2

Member

Rugvip Dec 17, 2024

Could you fill this out a little bit more please?

Contributor Author

gusega commented Dec 23, 2024

Hey guys, thanks a lot for the comments, I will take a look at them and update as soon as I am back from vacation (jan 6). I leave this comment here so that PR won't automatically close.

Contributor

github-actions bot commented Jan 7, 2025

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

github-actions bot added stale and removed stale labels


          updates after code review

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>

Contributor Author

gusega commented Jan 8, 2025

@Rugvip I updated the PR, please check.


          Merge branch 'backstage:master' into custom_auth_connector

gusega requested a review from Rugvip

January 9, 2025 09:16

gusega marked this pull request as draft

January 15, 2025 12:27


          Merge branch 'backstage:master' into custom_auth_connector

74a9cd5

Member

freben commented Jan 21, 2025

@gusega did you mark it as draft to indicate it's still being worked on and not ready for review?

Contributor Author

gusega commented Jan 21, 2025

@freben I want to add a unit test. I will mark it as ready after that.

Gasan Guseinov added 2 commits

January 23, 2025 18:37


          add a unit test

8a61795

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>


          remove unwanted changes

144f762

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>

Contributor Author

gusega commented Jan 23, 2025

I added a unit test to verify that I can create an OAuth2 with a custom auth connector.
fixed OAuth2CreateOptions export, previosly it was not exported
exported OAuth2Response
made OAuth2#normalizeScopes public
changed the implementation of OAuth2#create, now it accepts an options that is a union of OAuth2CreateOptions | OAuth2CreateOptionsWithAuthConnector and creates AuthConnector depending on the options provided. Previously authConnector was an optional property on OAuth2CreateOptions

Please review.

gusega marked this pull request as ready for review

January 23, 2025 17:47

gusega and others added 2 commits

January 24, 2025 09:58


          Merge branch 'backstage:master' into custom_auth_connector

c508d98


          update test name

af4f756

Signed-off-by: Gasan Guseinov <gasan.guseinov@ing.com>

Rugvip mentioned this pull request

🚀 Feature: Add client side (in-browser only) OIDC auth provider. #27584

Open

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

freben Awaiting requested review from freben freben is a code owner automatically assigned from backstage/maintainers

vinzscam Awaiting requested review from vinzscam vinzscam is a code owner automatically assigned from backstage/reviewers

Rugvip Awaiting requested review from Rugvip

Requested changes must be addressed to merge this pull request.

Labels

auth