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

Add cloud id resolver interface #35732

Closed
wants to merge 1 commit into from

Conversation

smesterheide
Copy link

@smesterheide smesterheide commented Dec 12, 2022

Summary

This is a companion/sub PR of #34132. An application requires group ids that contain slashes and colons. Full reasoning given:

Cloud Ids consist of username and remote parts. Usernames can also be group names. Federated groups are required to have "prefixes" to distinguish them from regular groups. Here group ids (gids) are written as Uniform Resource Names (URNs) that have colons and slashes as part of the uri scheme. We need to allow these characters as part of group names. Keep in mind that groups in Nextcloud can already contain these characters anyway.

Changes

  • Instead of validating all possible cloud ids there is now a mechanism to register cloud id resolver

TODO

  • ...

Checklist

@smesterheide
Copy link
Author

The obvious question to start is why are test:[email protected] or test/[email protected] considered invalid cloud ids while groups test:foo or test/foo may exists.

Comment on lines 112 to 125
$posSlash = strpos($id, '/');
$posColon = strpos($id, ':');

if ($posSlash === false && $posColon === false) {
$invalidPos = \strlen($id);
} elseif ($posSlash === false) {
$invalidPos = $posColon;
} elseif ($posColon === false) {
$invalidPos = $posSlash;
} else {
$invalidPos = min($posSlash, $posColon);
}

$lastValidAtPos = strrpos($id, '@', $invalidPos - strlen($id));
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s not even clear to me what this code is supposed to do. The strpos dance is quite complex.

@schiessle Do you remember why these characters were forbidden?

@PVince81 @blizzz @icewind1991 Do you think allowing these characters in here can lead to trouble? They are allowed in group names so it does make sense.
In which case I would vote for removing this whole block as invalid userids should be catched elsewhere and here we should just split on the @ sign.

Copy link
Member

@schiessle schiessle Dec 12, 2022

Choose a reason for hiding this comment

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

Puh, that's really a good question.

But whatever you do, keep in mind that you can't just split at the first @-sign. People can use email addresses as uids, so a valid federated cloud ID could look like user@mailserver@nextcloudserver. Maybe it would be safe to always split on the last @-sign but maybe I miss some other edge cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that’s what the strrpos is doing, finding the last @ sign.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the question for me is: why don't we just search for the last @-sign but have this offset? Do we break something when we remove the offset? My gut feeling says that it should be fine. But I hardly believe that we come up with all this logic to calculate the offset without a good reason. That's why I still fear that we miss something.

Can we find the pull request which introduced this code? Maybe the description or discussion gives us a idea why it was done this way?

Copy link
Author

Choose a reason for hiding this comment

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

I just want to give a short recount what $lastValidAtPos = strrpos($id, '@', $invalidPos - strlen($id)) does.

  • If there is no slash or colon, strrpos just returns the last position of at sign (or false).
  • Otherwise the search window for at sign is reduced by cutting off the part after the first occurrence of colon or slash (negative offset).

So user@https://example.com and [email protected]@https://example.com are both valid while urn:isbn:0451450523@https://example.com is not.

Maybe the reason for having this complex logic was to support URLs with userinfo like user@https://username:[email protected]. This would correctly identify the first at sign as lastValidAtPos while a simple strrpos($cloudId, '@') does not.

Copy link
Author

Choose a reason for hiding this comment

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

Can we find the pull request which introduced this code? Maybe the description or discussion gives us a idea why it was done this way?

This one might be relevant to the discussion: 2b7e5f8

Copy link
Author

@smesterheide smesterheide Feb 20, 2023

Choose a reason for hiding this comment

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

Instead of validating all possible cloud ids there is now a mechanism to register cloud id resolvers.

Validation has become more user-centric in 256fbe9.

@come-nc
Copy link
Contributor

come-nc commented Dec 12, 2022

Does anyone know if there is a documented limit of size for federated cloud id?
Where is the specification for that format anyway?

@PVince81
Copy link
Member

@come-nc there is likely a limit, either 512 or 255 chars, potentially shorter, see oc_share_external column "remote" and "remote_id"

@szaimen szaimen added this to the Nextcloud 26 milestone Dec 12, 2022
@szaimen szaimen added the 3. to review Waiting for reviews label Dec 12, 2022
@szaimen szaimen requested review from a team, PVince81 and icewind1991 and removed request for a team December 12, 2022 11:58
@blizzz blizzz mentioned this pull request Feb 1, 2023
@smesterheide smesterheide force-pushed the enh/extend-allowed-cloud-id-characters branch from a283542 to 2fe2427 Compare February 20, 2023 03:19
@smesterheide
Copy link
Author

oc_share_external.user does have a limitation of 64 characters. There is a migration in #34132 to increase it to 255 characters in order to support long group names.

@smesterheide smesterheide changed the title Allow slash and colon characters in cloud id Add cloud id resolver interface Feb 20, 2023
Comment on lines +46 to +54
/**
* Check if the input is a correctly formatted cloud id
*
* @param string $cloudId
* @return bool
*
* @since 26.0.0
*/
public function isValidCloudId(string $cloudId): bool;
Copy link
Author

Choose a reason for hiding this comment

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

If the resolver returns true it indicates to the CloudIdManager that this cloud id should also be resolved by it. This is a cursory check and an exception can still be thrown during resolveCloudId

Signed-off-by: Sandro Mesterheide <[email protected]>
@smesterheide smesterheide force-pushed the enh/extend-allowed-cloud-id-characters branch from 2fe2427 to 6deb049 Compare February 20, 2023 16:56
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
*
* @since 26.0.0
*/
public function registerCloudIdResolver(ICloudIdResolver $resolver);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function registerCloudIdResolver(ICloudIdResolver $resolver);
public function registerCloudIdResolver(ICloudIdResolver $resolver): void;

*
* @since 26.0.0
*/
public function unregisterCloudIdResolver(ICloudIdResolver $resolver);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function unregisterCloudIdResolver(ICloudIdResolver $resolver);
public function unregisterCloudIdResolver(ICloudIdResolver $resolver): void;

namespace OC\Federation;

use OCP\Federation\ICloudId;
namespace OCP\Federation;

class CloudId implements ICloudId {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class CloudId implements ICloudId {
/**
* @since 27.0.0
*/
class CloudId implements ICloudId {

Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* @param ICloudIdResolver $resolver
*
* @since 26.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @since 26.0.0
* @since 27.0.0

/**
* @param ICloudIdResolver $resolver
*
* @since 26.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @since 26.0.0
* @since 27.0.0

/**
* @param ICloudIdResolver $resolver
*/
public function unregisterCloudIdResolver(ICloudIdResolver $resolver) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function unregisterCloudIdResolver(ICloudIdResolver $resolver) {
public function unregisterCloudIdResolver(ICloudIdResolver $resolver): void {

/**
* @param ICloudIdResolver $resolver
*/
public function registerCloudIdResolver(ICloudIdResolver $resolver) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function registerCloudIdResolver(ICloudIdResolver $resolver) {
public function registerCloudIdResolver(ICloudIdResolver $resolver): void {

@@ -100,6 +104,12 @@ public function handleCardEvent(Event $event): void {
*/
public function resolveCloudId(string $cloudId): ICloudId {
// TODO magic here to get the url and user instead of just splitting on @

foreach ($this->cloudIdResolvers as $resolver) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if there is a better way, but isValidCloudId will be called twice on each resolver, since it is then called by $this->isValidCloudId. Not ideal performance-wise long-term.

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz removed this from the Nextcloud 27 milestone May 23, 2023
@blizzz blizzz added this to the Nextcloud 28 milestone May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@sorbaugh
Copy link
Contributor

Hello @smesterheide , looks like this PR was marked as "stale". But would like to reach out since there was already quite some work put here and would encourage you to ping me here to see if you'd like to reopen! 🚀

@skjnldsv skjnldsv deleted the enh/extend-allowed-cloud-id-characters branch August 16, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants