Skip to content

SASL PLAIN and anonymous authentication#130

Merged
Jokler merged 17 commits into
prototypefrom
31-sasl-plain-and-anonymous-authentication
Jul 21, 2026
Merged

SASL PLAIN and anonymous authentication#130
Jokler merged 17 commits into
prototypefrom
31-sasl-plain-and-anonymous-authentication

Conversation

@Jokler

@Jokler Jokler commented Jul 17, 2026

Copy link
Copy Markdown
Member

This PR fixes the following divergences between the prototype branch and #31:

Additions after review:

  • Improve SASL flow to match the spec better
  • Fix bug in chunking
  • Bubble up the SASL error text
  • Set the network name in ServerMetadata from ISUPPORT
  • Log anyhow error chain and bubble up the top level error as a String
  • Return sign in User/Guest status after sign_up is called

Resolves: #31

@Jokler
Jokler requested a review from zealsprince July 17, 2026 14:44
@Jokler Jokler added the uplink The IRC layer: protocol, caps, messaging, metadata label Jul 17, 2026
@Jokler Jokler added this to Orbit Jul 17, 2026
@Jokler

Jokler commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

I'll leave fixing that build error to @dolanske because reproducing it locally is blocked by other errors on my system.

@zealsprince zealsprince left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall I like where this landed. The typed OrbitError is a real improvement over stringly errors at the boundary, and the .context() pass means failures will actually be traceable now. Left inline comments, the two I'd want sorted before merge are the split_at panic and the question of waiting for the AUTHENTICATE challenge. The silent fallback I'd at least want a shared understanding on. The UI surfacing can be a follow-up issue.

A few things that didn't fit inline:

  • The lockfile looks half-updated: catalog says vite-plus ^0.2.4 but the catalogs section resolves 0.2.1, root gets 0.2.4, and the apps stay on 0.2.1. Does a fresh pnpm install settle it for you?
  • The @ts-ignores on defineConfig, is that a vite-plus 0.2.4 type regression? If so I'd swap to @ts-expect-error with the upstream issue linked so they self-report when fixed.
  • The wasm-pack block is copy-pasted between ci.yml and cd.yml and runs cold each time. Composite action plus caching once the sourcing questions are answered?

Curious about the nightly requirement too, is that just for the if-let match guards or is more coming that needs it?

Also, we should look to resolve the CI errors with @dolanske so that we can get a nice clean PR out there :)

Comment thread .github/workflows/cd.yml Outdated
Comment thread .github/workflows/cd.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread packages/core/core-shared/src/actor.rs Outdated
Comment thread packages/core/core-shared/src/actor.rs Outdated
Comment thread packages/core/core-shared/src/send_command.rs
Comment thread packages/core/core-shared/src/state.rs
Comment thread packages/core/core-shared/src/state.rs
Comment thread packages/core/core-wasm/src/lib.rs Outdated
Comment thread packages/core/core-wasm/src/lib.rs
@Jokler
Jokler force-pushed the 31-sasl-plain-and-anonymous-authentication branch 2 times, most recently from f6a7493 to 7f0e46f Compare July 17, 2026 19:21
@Jokler
Jokler force-pushed the 31-sasl-plain-and-anonymous-authentication branch from 7f0e46f to de7df8a Compare July 17, 2026 19:23
@Jokler

Jokler commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

The lockfile looks half-updated: catalog says vite-plus ^0.2.4 but the catalogs section resolves 0.2.1, root gets 0.2.4, and the apps stay on 0.2.1. Does a fresh pnpm install settle it for you?

That printed this, but didn't actually change anything:

devDependencies:
- vite-plus 0.2.1
+ vite-plus 0.2.4

The @ts-ignores on defineConfig, is that a vite-plus 0.2.4 type regression? If so I'd swap to @ts-expect-error with the upstream issue linked so they self-report when fixed.

I don't know what's causing it, I actually updated vite-plus in the hopes of fixing that issue. @dolanske suggested to ignore the problem for now so maybe he can try to look into it once he is available.

The wasm-pack block is copy-pasted between ci.yml and cd.yml and runs cold each time. Composite action plus caching once the sourcing questions are answered?

Sounds good but I don't want to implement that in this PR.

Curious about the nightly requirement too, is that just for the if-let match guards or is more coming that needs it?

There is no nighly requirement, I have removed it. :)

@Jokler
Jokler requested a review from zealsprince July 17, 2026 21:35
use core_shared::actor::{ActorCommand, ActorMessage, IrcActor};
use futures::channel::oneshot;

const LONG_PASSWORD: &str = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lol

@zealsprince zealsprince left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good with your feedback! I'll capture the caching concern for the packages as a separate ticket!

@zealsprince zealsprince changed the title SASL PLAIN and anonymous authentication #31 SASL PLAIN and anonymous authentication Jul 18, 2026
@zealsprince zealsprince linked an issue Jul 18, 2026 that may be closed by this pull request
@zealsprince zealsprince changed the title #31 SASL PLAIN and anonymous authentication SASL PLAIN and anonymous authentication Jul 18, 2026
@zealsprince zealsprince moved this to In review in Orbit Jul 18, 2026
@Jokler
Jokler requested a review from dolanske July 21, 2026 13:01
@Jokler
Jokler merged commit 277fcd9 into prototype Jul 21, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Orbit Jul 21, 2026
@Jokler
Jokler deleted the 31-sasl-plain-and-anonymous-authentication branch July 21, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

uplink The IRC layer: protocol, caps, messaging, metadata

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

SASL PLAIN and ANONYMOUS authentication

3 participants