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

Use fetch middlewares in event-source-polifyll (re: #23015) #24820

Closed
wants to merge 4 commits into from

Conversation

alexef
Copy link
Contributor

@alexef alexef commented May 17, 2024

PoC to validate this would be the way forward in #23015 (scaffolder, techdocs use event-source-polyfill library that only allows headers to be passed but not the entire fetch implementation)

✔️ 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)

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@github-actions github-actions bot added the area:techdocs Related to the TechDocs Project Area label May 17, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented May 17, 2024

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage/core-app-api
  • @backstage/core-plugin-api
  • @backstage/test-utils
  • @backstage/plugin-scaffolder
  • @backstage/plugin-techdocs

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/core-app-api packages/core-app-api none v1.12.5
@backstage/core-plugin-api packages/core-plugin-api none v1.9.2
@backstage/test-utils packages/test-utils none v1.5.5
@backstage/plugin-scaffolder plugins/scaffolder none v1.20.0
@backstage/plugin-techdocs plugins/techdocs none v1.10.5

@alexef
Copy link
Contributor Author

alexef commented May 17, 2024

@aramissennyeydd / @freben pulling you from the original ticket, can you have a look and see if this would be an acceptable approach?

I can implement it as an eventSourceApi as you suggested, but I wouldn't want to duplicate the header.name configuration in FetchMiddlewares.injectIdentityAuth.

Not yet sure, but I assume if the new eventSourceApi depended on the fetchApi, then it could call the new fetchApi.headers? method so the configuration is done only once.

My current code looks like this:

createApiFactory({
    api: fetchApiRef,
    deps: {
      configApi: configApiRef,
      identityApi: identityApiRef,
      discoveryApi: discoveryApiRef,
    },
    factory: ({ configApi, identityApi, discoveryApi }) => {
      return createFetchApi({
        middleware: [
          FetchMiddlewares.resolvePluginProtocol({
            discoveryApi,
          }),
          FetchMiddlewares.injectIdentityAuth({
            identityApi,
            config: configApi,
            header: {
              name: 'Backstage-Authorization',
              value: (backstageToken) => `Bearer ${backstageToken}`,
            }
          }),
        ],
      });
    },
  }),

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@@ -130,15 +126,14 @@

expect(MockedEventSource).toHaveBeenCalledWith(
'http://backstage:9191/api/techdocs/sync/default/Component/test-component',
{ withCredentials: true, headers: { Authorization: 'Bearer token' } },
{ withCredentials: true, headers: { authorization: 'Bearer token' } },

Check failure

Code scanning / CodeQL

Hard-coded credentials Critical

The hard-coded value "Bearer token" is used as
authorization header
.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a test

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@github-actions github-actions bot added the area:scaffolder Everything and all things related to the scaffolder project area label May 17, 2024
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@Rugvip
Copy link
Member

Rugvip commented May 17, 2024

Thank you! 👍

Wanna note here as well that I think we should explore #23015 (comment) before making additions to the core APIs

@alexef
Copy link
Contributor Author

alexef commented May 22, 2024

@Rugvip please check #24861 which should replace this PR if viable.

@alexef
Copy link
Contributor Author

alexef commented May 22, 2024

closing in favor of: #24861

@alexef alexef closed this May 22, 2024
Copy link
Contributor

Uffizzi Cluster pr-24820 was deleted.

@alexef alexef deleted the fetch-middleware-events branch May 22, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scaffolder Everything and all things related to the scaffolder project area area:techdocs Related to the TechDocs Project Area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants