auth-node: refactor OAuth scope management #24743
Conversation
Changed Packages
|
Rugvip
added
merge-after-release
|
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! |
|
Hi @Rugvip , I wonder is that planned in this PR to remove some duplicated |
|
@liununu yep that's one of the goals of this refactor. Some of it will be in followup PRs though |
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
|
There are quite a lot of reports coming in of issues that are fixed by this PR. I was aiming to have these changes go out a bit slower and per provider, but since this fixes a lot of known issues I think we're better off shipping this a bit faster and aim to have it put us in a better state than we are now. I've updated existing providers in 8efc6cf so this PR is not ready to go with hopefully not breakages of existing providers other than intentional ones. |
Rugvip
force-pushed
the
rugvip/scopes
branch
3 times, most recently
from
ede645b
to
4fe0416
Compare
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
|
@liununu to follow up a bit more, it's not the goal of this PR to completely remove the default scopes in the frontend. We're keeping them around to ensure compatibility as these changes are rolled out. In theory it would be safe to remove them straight away based on our version skew policy, but in practice it's nice to give a bit of leeway where possible to avoid breaking deployments it tricky ways. I'll follow up on this work in a later release to start consolidating the auth APIs in the frontend and remove duplicated declarations. |
| @@ -68,7 +65,7 @@ describe('authModuleOktaProvider', () => { | |||
| expect(startUrl.pathname).toBe('/oauth2/v1/authorize'); | |||
| expect(Object.fromEntries(startUrl.searchParams)).toEqual({ | |||
| response_type: 'code', | |||
| scope: combinedScopes, | |||
| scope: 'openid email profile offline_access groups phone', | |||
There was a problem hiding this comment.
With the latest updates this now properly merges scopes again, regarding #24743 (comment)
|
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
This standardizes scope management across OAuth providers, and also aims to fix a couple of issues, especially providers that persist scopes. The overall goal of this is to be able to completely remove the need for bespoke auth APIs in the frontend, in particular for sign-in.
One issue that's been fixed is refreshing with scope persistence. In the current implementation the persisted scopes will always be used, which can break this client flow where a session is refreshed with requested scopes.
An issue that I'll be aiming to fix in followup usages of this is that many auth providers pass the
scopeoption to the passport strategy. That only works if no scopes are requested by the client, because they are not merged. This aims to fix that by properly merging together required, additional, requested and granted scopes.Will do updates for each provider in followups, so that we can get some eyes/testing of each individual update.
✔️ Checklist
Signed-off-byline in the message. (more info)