Skip to content

Conversation

@avdb13
Copy link

@avdb13 avdb13 commented Dec 18, 2024

This is a draft, a few things to consider:

  • Should email verification be allowed through authifier or only through the upstream ID provider?

Provide the following options:

  • native: the account has to verify its email via revolt
  • oauth: the account has to verify its email via the upstream provider, we check this with the email_verified claim (if not present? should default to the former option? or just error at startup? )
  • native-oauth: the same as the previous option without the requirement of the email_verified claim
  • disabled: just mark all oauth users as having their email verified
  • Should the encoding secret be configurable?

I suppose the secret should just be configurable then so it's both persistent and private

  • How specific do we want the error variants to be?

...

  • Do we wanna store the metadata of OIDC providers? If so how long should they be cached for?

Yes, cache it for min. 1 hour

All I can think of for now. Refer to https://connect2id.com/learn/openid-connect and https://connect2id.com/learn/oauth-2-1 for implementation details.

@avdb13 avdb13 marked this pull request as draft January 22, 2025 10:15
@avdb13 avdb13 force-pushed the single-sign-on branch 3 times, most recently from aec1658 to d383af1 Compare March 22, 2025 00:33
pub scopes: Vec<String>,
pub endpoints: Endpoints,
pub credentials: Credentials,
pub claims: HashMap<Claim, String>,
Copy link
Author

Choose a reason for hiding this comment

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

This allows flexibility for claims, claims are a hashmap of information about the user, returned in the token or userinfo response i.e. ("preferred_email" -> "[email protected]", etc.) and this will allow cases where we wanna point out that "preferred_email" corresponds to the Email claim in case of unconventional keys.

pub credentials: Credentials,
pub claims: HashMap<Claim, String>,

pub code_challenge: bool,
Copy link
Author

Choose a reason for hiding this comment

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

pub code_challenge: bool,
}

impl Borrow<str> for IdProvider {
Copy link
Author

Choose a reason for hiding this comment

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

The following impls are defined in order to prevent ID providers with duplicate IDs in the configuration.

) -> Result<Option<Account>>;

/// Find account by SSO ID
async fn find_account_by_sso_id(&self, idp_id: &str, sub_id: &str) -> Result<Option<Account>>;
Copy link
Author

Choose a reason for hiding this comment

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

Every token/userinfo response should have a subject claim which can be utilized to identify the user, in this case we need to know the idp_id i.e. Google/GitHub/etc. and the sub_id (should be serde_json::Value).

@avdb13 avdb13 marked this pull request as ready for review June 10, 2025 07:30
}

#[derive(Debug, Default, Clone, PartialEq, Eq)]
pub struct SSO(HashSet<IdProvider>);
Copy link
Member

@Zomatree Zomatree Nov 13, 2025

Choose a reason for hiding this comment

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

Is there a reason this isnt a HashMap? would remove the need for a custom Serialize, Deserialize and Hash impl

Suggested change
pub struct SSO(HashSet<IdProvider>);
pub struct SSO(pub HashSet<IdProvider>);


/// Save callback
async fn save_callback(&self, callback: &Callback) -> Success {
self.collection::<Callback>("callbacks")
Copy link
Member

Choose a reason for hiding this comment

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

This needs upsert to be set to true otherwise it will never create anything.


/// Save secret
async fn save_secret(&self, secret: &Secret) -> Success {
self.collection::<Secret>("secret")
Copy link
Member

Choose a reason for hiding this comment

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

See before about upsert, however it would be better if this is hardcoded via a config value instead of being auto-generated and stored in the database

.expect("server must have a URL");

let callback_url = format!(
"https://{}/auth/sso/callback",
Copy link
Member

Choose a reason for hiding this comment

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

Its probably worth switching to using the host instead of the domain, and allowing ports and path.

@Zomatree
Copy link
Member

If you are no longer wanting to work on this - i know its been a while, im happy to pick this up and finish it off, please let me know.

@avdb13
Copy link
Author

avdb13 commented Nov 14, 2025

If you are no longer wanting to work on this - i know its been a while, im happy to pick this up and finish it off, please let me know.

Go ahead, there is very little work left, mostly regarding the changes you suggested.

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