Skip to content

FFI: Add support for OIDC login hints#5012

Merged
poljar merged 2 commits intomainfrom
doug/login-hint
May 8, 2025
Merged

FFI: Add support for OIDC login hints#5012
poljar merged 2 commits intomainfrom
doug/login-hint

Conversation

@pixlwave
Copy link
Member

@pixlwave pixlwave commented May 7, 2025

This PR makes 2 changes:

@pixlwave pixlwave requested a review from a team as a code owner May 7, 2025 11:21
@pixlwave pixlwave requested review from poljar and removed request for a team May 7, 2025 11:21
@codecov
Copy link

codecov bot commented May 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.85%. Comparing base (b5b2450) to head (36c7747).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...-sdk/src/authentication/oauth/auth_code_builder.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5012      +/-   ##
==========================================
- Coverage   85.87%   85.85%   -0.02%     
==========================================
  Files         325      325              
  Lines       35853    35856       +3     
==========================================
- Hits        30787    30784       -3     
- Misses       5066     5072       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +76 to +81
/// Set a generic login hint to help an identity provider pre-fill the login
/// form.
///
/// Note: This is not the same as the [`Self::user_id_hint()`] method, which
/// is specifically designed to a) take a `UserId` and no other type of
/// hint and b) be used directly by MAS and not the identity provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this overwrites whatever we set with user_id_hint(), we should mention this.

Out of interest, what will you set as the hint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I've added the same The following methods are mutually exclusive line that we use in the client builder.

It will likely be an email address but it depends on the upstream provider, we would pass through whatever is given to us without modifying it. The use case is an organisation creating provisioning links like https://app.example.com/?server_name=example.org&login_hint=alice where their upstream provider knows how to handle alice as a hint.

There are docs here: https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah makes sense. Can we perhaps more explicitly mention this example use-case and link to these docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep makes sense, done in 079285d

@pixlwave pixlwave force-pushed the doug/login-hint branch 2 times, most recently from 780d349 to 2c92db5 Compare May 7, 2025 14:50
@pixlwave pixlwave requested a review from poljar May 7, 2025 14:50
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

@poljar
Copy link
Contributor

poljar commented May 7, 2025

Ah, the CI isn't happy yet.

@pixlwave pixlwave force-pushed the doug/login-hint branch from 2c92db5 to 36c7747 Compare May 7, 2025 15:02
@pixlwave
Copy link
Member Author

pixlwave commented May 7, 2025

I don't understand what the failure is:

❌ TestAliceBobEncryptionWorks/{rust_hs1}|{rust_hs1} (0s)
  2025/05/07 15:07:48 Sharing [SERVER_NAME=hs1 SYNAPSE_COMPLEMENT_DATABASE=sqlite] host environment variables with container
      test_package.go:183: CreateDirtyDeployment failed: CreateDirtyServer: Failed to deploy image homeserver : Error response from daemon: Conflict. The container name "/complement_crypto_dirty_hs1" is already in use by container "fe7d8290369fd8f61878229944b863e7b67ab074c1f6bfb67228ed36e9945b1c". You have to remove (or rename) that container to be able to reuse that name.

@pixlwave
Copy link
Member Author

pixlwave commented May 7, 2025

Ah seems like this is hitting other PRs and the fix is here: matrix-org/complement-crypto#174 matrix-org/complement-crypto#175

@poljar poljar merged commit be6d5f9 into main May 8, 2025
42 checks passed
@poljar poljar deleted the doug/login-hint branch May 8, 2025 10:10
@MatMaul
Copy link
Contributor

MatMaul commented May 9, 2025

Thanks a lot for that and the corresponding bits in MAS, you beat us to it by a week or 2 I believe :)

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.

3 participants