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

App owned logins #330

Merged
merged 2 commits into from
Dec 9, 2021
Merged

App owned logins #330

merged 2 commits into from
Dec 9, 2021

Conversation

walkah
Copy link
Contributor

@walkah walkah commented Nov 30, 2021

This PR introduces dependency injection for initialization and account registration (along with the various username validation features). This allows apps to override these and potentially use alternative authentication backends / mechanisms.

It also introduces a "local" version that allows an app to init/register accounts in a self-contained manner like this:

import { LOCAL_IMPLEMENTATION } from "webnative/auth/local";
setDependencies(LOCAL_IMPLEMENTATION);
wn.initialise({ loadFileSystem: false })
  .then((result) => {
    const { authenticated, username } = result;
    if (authenticated) {
      console.log(`Logged in as: ${username}`)
    }
  })
  .catch((e) => console.error(e));

Closes #316
Closes #302

@walkah walkah marked this pull request as ready for review December 8, 2021 21:47
@walkah walkah requested a review from matheus23 December 8, 2021 21:47
Copy link
Contributor

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Minor stuff only & a small question. Otherwise looking good to me :)
tumbsup

import { USERNAME_STORAGE_KEY } from "../common/index.js"

export const init = async (): Promise<State | null> => {
console.log("initialize local auth")
Copy link
Contributor

Choose a reason for hiding this comment

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

We could switch out console.log with the debug logging functions, so they only print with setup.debug({ enabled: true })

Copy link
Member

Choose a reason for hiding this comment

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

setup.debug({ enabled: true })

This is also generally togglable in the console directly — no need for managing it ourselves

Screen Shot 2021-12-09 at 10 01 27

Comment on lines +86 to +87
isUsernameValid: (username: string) => Promise<boolean>
isUsernameAvailable: (username: string) => Promise<boolean>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a Q for clarification: Why do we abstract these functions? We're using the same implementation for both local auth as well as auth-lobby based auth it seems.
Is this so that someone would in principle be able to implement their own "username database"? If so, is that possible today? I think we'd need email-like identifiers for that, i.e. something like matheus23@fission.codes usernames instead of matheus23. E.g. in the DNSLink fetching code.
Maybe it doesn't hurt having these abstractions in here today already, but I'm not sure. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is so that you can plug in any arbitrary lookup method, rather than forcing everyone to make it look like Fission's. For example, web3.storage has this in a big database table that's at a different endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

RE validation: services may have special validation rules for their service (e.g. no curse words in Spanish). I believe that the plan is to provide defaults and let users override them if needed.

something like matheus23@fission.codes usernames instead of matheus23

That's more of an interoperability question. Blaine calls this style the "Jesus of Nazareth" format, and I agree that it reads very well (in English at least)

Copy link
Member

Choose a reason for hiding this comment

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

(But per service, you can imply the host, in the same way that Discord hides the nonce in your username unless there's a collision on that server)

Copy link
Member

@expede expede left a comment

Choose a reason for hiding this comment

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

LGTM! Bump the version and...

@walkah walkah merged commit 21a04f8 into main Dec 9, 2021
@walkah walkah deleted the app-owned-logins branch December 9, 2021 17:42
@bgins bgins mentioned this pull request May 4, 2022
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.

Generalize Username Resolution Move account creation to webnative
3 participants