Add custom email mapping support

[lvaroqui]

Work in progress

Fixes #872

[lvaroqui]

Hey @sebadob, what do you think about the UI I added for user_attr type?

Also I faced an issue:

When serializing typ, serde uses PascalCase, so Email, however the as_str implementation was transforming it to email. This difference between serialization and de-serialization caused the users/attr route to not return attrs with their attribute set (I'm guessing because serde could not convert email to UserAttrConfigTyp::Email.

I chose to use Email across the board so that we do not have to do handle conversions. But I can do something else if you want!

This is not that bad but the real issue is that I did not get any warning / error on backend logs when that happened.

[lvaroqui]

After a bit of digging, I think the issue lies in hiqlite: https://github.com/sebadob/hiqlite/blob/main/hiqlite/src/query/mod.rs#L168

[sebadob]

what do you think about the UI I added for user_attr type

Very limited on time today, can take a look tomorrow.

When serializing typ, serde uses PascalCase, so Email, however the as_str implementation was transforming it to email

I would probably tell serde to convert it properly via #[serde(rename_all = "lowercase")]. Hiqlite just takes the values as they are. If you use query_as() it will use serde under the hood and as long as serde can properly deserialize, it's all good.

But, I already added the From<_> impl for UserAttrConfigEntity and it works like expected. This is only used for query_map() and not for query_as() fns.

Edit:

Btw regarding the rocksdb issue with your most probably gcc 15, The unstable ref to the github repo cannot stay for a release. The final release compilation does not have this issue, as it's not using gcc 15 inside the builder container. Locally, you could either do a quick an dirty patch to your cached registry like shown here, or use export CXXFLAGS="$CXXFLAGS -include cstdint".

[lvaroqui]

Very limited on time today, can take a look tomorrow.

No rush ahah, you're already more responsive than 95% of open source maintainers 😄

I would probably tell serde to convert it properly via #[serde(rename_all = "lowercase")].

I'll fix on serde side then, no problem!

rocksdb

Concerning the rocksdb, of course I was planning to remove the commit when the PR is ready, should have mentioned that 😄 I'll use export CXXFLAGS="$CXXFLAGS -include cstdint"

[sebadob]

what do you think about the UI I added for user_attr type?

That's what I had in mind. Just a selector with pre-defined types.

Instead of having the button there, the default could maybe just be - and set the value to undefined if this matches. This would be an even better UX I would say.

And in case of email, when there is additional validation necessary in the backend, we should probably show a small text mentioning how it works.

[sebadob]

I just merged a very huge PR (#991) and you luckliy have no conflict yet, but you should probably pull the changes to make sure you don't get into trouble when doing templates.

Also, I tested a lot, and it seems fine to me so far, but if you have any configuration issues, please let me know. I cleaned up the whole config situation which spiraled a bit ouf of control and got messy, and I ended up having some static vars here and there all over the place. Now it's very clean, but almost 12k lines changed.

[lvaroqui]

Hi @sebadob,

I got something working and basic UI setup, I'd be glad if you could take a look and tell me if my implementation is to your liking 🙂

Things I still need to do:

• Add check of custom_email_mapping attr condition (disallow user editable attribute and only typ email)
  • On the backend
  • On the frontend
• Add explanatory text on the frontend in client custom_email_mapping section

What's the best way to do the checks on the backend? How do we ensure that an attr is not modified if it is used in a custom_email_mapping in client?

Do you see anything else?

[sebadob]

You probably need to bump the index on these files by 1 because of the recently added login_locations migrations. Also, a newline at the end would be nice. Not because it would be necessary in this case, but it often is and I am used to always add one, no matter in which context.

[sebadob]

Do you see anything else?

Actually, yes. We should think about a confirmation on the user side if an additional E-Mail is being added, very much like the flow when a user changes the primary email via account dashboard. Notifications are sent to both the old and new addresses, and the user needs to confirm that change, before it's being finally executed. Most of the logic can be re-used. Probably even the E-Mail templates, but with slightly adjusted text. Something like

The E-Mail address alfread@batcave.io is being added as a secondary E-Mail for the Rauthy account batman@batcace.io. To accept this, please click the link below.

Clicking the magic link then should make the change final. This E-Mail should be sent to the new address, that should be added. In addition, a notification about this, but without the link, should probably be sent to the current primary address.

---

I had some more thoughts about this and if we allow an admin to do this change without any confirmation, it would be pretty easy for a malicious admin to take over complete accounts even without the knowledge of the user. For instance, you could add another users address as your own secondary and login to Google Workspace in this case, and you could impersonate the other one quietly. The bare minimum that should be done is sending out a notification about the change to both addresses. However, triggering the flow in the same way as an email change is being executed right now is not that big of an overhead. The magic link creation to finally execute this should be possible without any other database modifications.

Edit:

It could be, that I have some spare time to help with this email template thingy in the next days, so that you only need to trigger it. I will let you know.

[lvaroqui]

For your second point about a "malicious admin", is that really a concern we can address, to me if the admin is malicious he will be able to impersonate any one he wants. Actually in other SSO solutions (eg. Authentik), there's an impersonate button to exactly do that in the admin UI!

[sebadob]

(...) to me if the admin is malicious he will be able to impersonate any one he wants.

That's not the case for Rauthy. In fact, that is one of the reasons why impersonate does not exist on purpose, like it does in Authentik for instance. I thought about something like "token inspection" to be able to see how a token would look like for a specific client with the current configuration, but even in that case, I would never issue an actually valid token.

If you run Rauthy, you could only do harm to users sub-accounts if you are also root on the server or storage below, but a rauthy_admin cannot do this. It can delete your account, or remove MFA keys and then set a new password for your account, to log in with that, but the user will be notified about that (at least in probably the next version).

It should at least not be easy to do this, even as an admin. User impersonation is something that Enterprises may want on certain platforms for good reason, but doing that on the IdP allows so much access, even without the user knowing about it. But yeah that could be a 2nd step at least, to add more safeguards.

[lvaroqui]

Got it, makes sense!

Quick question, what is the better approach to do the validation that a custom_email_mapping is email and read-only as this has to be checked when we edit clients but also when we edit the attribute if it used somewhere as a mapping? My go to would have been a database constraint?

[sebadob]

Quick question, what is the better approach to do the validation that a custom_email_mapping is email and read-only as this has to be checked when we edit clients but also when we edit the attribute if it used somewhere as a mapping? My go to would have been a database constraint?

That is not too straight forward. It has to be checked in multiple places and show a nice error message to the user, if this is the case.

I would of course check it for PUT /client. While thinking about it, I would maybe change the type def for custom attrs to the creation phase and make them immutable afterward. This makes sure that an email cannot be changed later on and it's a nice additional guarantee for client apps that types will not suddenly change.
Something else that could be added is a new column for custom attrs, which marks them as locked. This could be set to true when a client mapping is being created (and of course unlocked if it's removed). When such a value is locked, it's also a prevention that it can never be made user-editable. This could be shown nicely in the UI by either not showing the button, or better showing an indicator that it's locked. The locked state can then also be checked in the PUT for the attr config.

Having additional DB columns is a bit more work on the client side, but makes everything a lot clearer in the UI. Not sure how you would do all of that with a DB constraint, but if you have a solution that works, sure why not.

[lvaroqui]

Pinning the type of the attrs seems like a good idea, that's one less way for the users to break stuff!

The idea of the locked seems good, but making it a column is a little strange to me as the information can be derived with a join on the clients table by simply checking that the attribute is used as a custom mapping.

With a DB constraint, or maybe a trigger, you can make constraint on a column based on a custom querry, this would allow the DB to stay consistant by itself, raising an error if an attempt at a bad modification is done. This would avoid having to do the checks everywhere a modification is done to either clients or attrs. Haven't done that in a while though but maybe worth exploring.

I tough the prettiness of the backend error would not be too big of a concern since on the client side we won't allow to do the invalid request anyway, so a DB error didn't seem too bad.

[sebadob]

The idea of the locked seems good, but making it a column is a little strange to me as the information can be derived with a join on the clients table by simply checking that the attribute is used as a custom mapping.

We don't need this information in the auth chain and since it's only necessary when modifying via Admin UI or updating, that's fine as well, sure.

With a DB constraint, or maybe a trigger, you can make constraint on a column based on a custom querry, this would allow the DB to stay consistant by itself, raising an error if an attempt at a bad modification is done.

Even though this will probably not be an issue with Rauthy, I really don't like triggers and custom functions on the DB, because it requires more resources and time being spent in a place that is usually the bottleneck when you need to scale.
Rauthy is also the only app accessing the DB and Rust is already strictly type-checked, which is the reason why I keep DB definitions pretty loose as well and e.g. don't set limits on VARCHARS and things like that. I even avoid JOINs when it's possible to move compue intense stuff into the app logic, if it makes sense. In this case though I would do a join, yes. It requires less maintenance and manual work.

Btw all of this is also the reason why the roles, groups and scopes are handled against everything you "learn in school" about DB normalization and stuff. These values are rarely updated and if I can pre-compute stuff and therefore have less stress on the DB in the hot path, I always do it, even if it means a bit more manual maintenance.

I tough the prettiness of the backend error would not be too big of a concern since on the client side we won't allow to do the invalid request anyway, so a DB error didn't seem too bad.

Easily solved as well, when you JOIN the data that is returned to the Admin UI and dynamically resolve the locked information. Or you could also simply fetch all clients in the Admin UI and do it there, if you find that easier. At that point, the overhead is really not an issue.