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

Implement OpenID add/remove accounts in identity management #2762

Merged
merged 14 commits into from
Jan 3, 2025

Conversation

sea-snake
Copy link
Contributor

@sea-snake sea-snake commented Dec 31, 2024

Implement OpenID add/remove accounts in identity management

Changes

  • Add OPENID_AUTHENTICATION feature flag
  • Add II_OPENID_GOOGLE_CLIENT_ID environment variable
  • Copy over redirect in popup implementation from prototype
  • Copy over request JWT implementation from prototype
  • Implement "Linked Accounts" section to Identity manage (behind above flag)
  • Implement add/remove account with mock actor and request JWT method
  • Implement hasOtherAuthMethods to allow removal of all passkeys/credentials if one still has credentials/keys., since a user only needs to have at least 1 passkey or linked account.
image
🟢 Some screens were added
🟡 Some screens were changed

@sea-snake sea-snake requested a review from lmuntaner December 31, 2024 12:52
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks! Could you also provide some screenshots?

A few comments and questions. I like where this is going!

package.json Show resolved Hide resolved
@@ -433,7 +433,7 @@ xr-spatial-tracking=()",
let rgx = Regex::new(
"^default-src 'none';\
connect-src 'self' https:;\
img-src 'self' data:;\
img-src 'self' data: https://\\*.googleusercontent.com;\
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this to be able to show the image from the google account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, currently the CSP only allowed images from it's own canister and data uris. I considered fetching the image and storing it as datauri instead but that would add quite some complexity and prevent it from showing the latest image if the user changes their profile image.

Copy link
Collaborator

@lmuntaner lmuntaner Jan 2, 2025

Choose a reason for hiding this comment

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

Yes, this is fine. Maybe check with security by sending them a message, but it't not blocking this PR.

src/frontend/src/featureFlags/index.ts Show resolved Hide resolved
src/frontend/src/flows/manage/index.ts Show resolved Hide resolved
src/frontend/src/flows/manage/index.ts Outdated Show resolved Hide resolved
src/frontend/src/utils/mockOpenID.ts Show resolved Hide resolved
src/frontend/src/utils/mockOpenID.ts Show resolved Hide resolved
src/frontend/src/utils/iiConnection.ts Show resolved Hide resolved
src/frontend/src/styles/main.css Show resolved Hide resolved
src/frontend/src/index.ts Outdated Show resolved Hide resolved
src/frontend/src/flows/redirect.ts Show resolved Hide resolved
src/frontend/src/utils/openID.ts Outdated Show resolved Hide resolved
src/frontend/src/utils/openID.ts Outdated Show resolved Hide resolved
src/frontend/src/utils/openID.ts Outdated Show resolved Hide resolved
src/frontend/src/utils/mockOpenID.ts Show resolved Hide resolved
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks! A couple more comments but already approving!

@sea-snake sea-snake added this pull request to the merge queue Jan 3, 2025
Merged via the queue into main with commit 0d59af9 Jan 3, 2025
68 checks passed
@sea-snake sea-snake deleted the sea-snake/list-linked-accounts branch January 3, 2025 14:21
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.

2 participants