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 option to save SCRAM details on successful login and document SCRAM login #3001

Closed
based-a-tron opened this issue Aug 10, 2022 · 7 comments
Labels

Comments

@based-a-tron
Copy link

@jcbrand Thank you very much for merging the SCRAM updates for Strophe.js!
The next logical step would be to add an option here to allow users to save their SCRAM data on login.
As discussed in the strophe pull, users can submit a password object of the form

{ name: String, representing the hash used (eg. SHA-1),
  salt: String, base64 encoded salt used to derive the client key,
  iter: Int,       the iteration count used to derive the client key,
  ck:   String, the base64 encoding of the SCRAM client key
  sk:   String, the base64 encoding of the SCRAM server key
}

in the normal password field now. This should probably be documented somewhere, or maybe abstracted over a bit for converse web administrators to use without having to know what a scram key is.

My users and I have been running a hacky client to accomplish this for some weeks now, where we save the data to browserstorage in https://github.com/conversejs/converse.js/blob/master/src/headless/utils/init.js after successful login, like so:

function connect (credentials) {
    const { api } = _converse;
    if ([_converse.ANONYMOUS, _converse.EXTERNAL].includes(api.settings.get("authentication"))) {
        if (!_converse.jid) {
            throw new Error("Config Error: when using anonymous login " +
                "you need to provide the server's domain via the 'jid' option. " +
                "Either when calling converse.initialize, or when calling " +
                "_converse.api.user.login.");
        }
        if (!_converse.connection.reconnecting) {
            _converse.connection.reset();
        }
        _converse.connection.connect(_converse.jid.toLowerCase());
    } else if (api.settings.get("authentication") === _converse.LOGIN) {
        const password = credentials?.password ?? (_converse.connection?.pass || api.settings.get("password"));
        if (!password) {
            if (api.settings.get("auto_login")) {
                throw new Error("autoLogin: If you use auto_login and "+
                    "authentication='login' then you also need to provide a password.");
            }
            _converse.connection.setDisconnectionCause(Strophe.Status.AUTHFAIL, undefined, true);
            api.connection.disconnect();
            return;
        }
        if (!_converse.connection.reconnecting) {
            _converse.connection.reset();
            _converse.connection.service = getConnectionServiceURL();
        }

        let callback;

        if (api.settings.get("save_scram_keys") && !password?.sk) {
            // Don't save if a scram key password was already supplied
            console.log("Attempting to save scram keys...");
            callback = (status) => {
                // Save scram keys in localstorage
                const scramKeys = _converse.connection.scram_keys;
                if (scramKeys) {
                    try { 
                        localStorage.setItem('scram_keys', JSON.stringify(scramKeys));
                        localStorage.setItem('scram_login', _converse.connection.authzid);
                    } catch (e) { // Could not find local storage }
                        console.log("No storage method found.");
                    }
                }
                _converse.connection.onConnectStatusChanged(status);
            };
        }

        _converse.connection.connect(_converse.jid, password, callback);
    }
}

Where naturally there is a new setting called save_scram_keys.
When auto_login is specified, the saved scram data should (probably?) be used by default if no password field is specified.
This is a rather sloppy way to do it, and I'm sure you'd like to do it in a more elegant way. Regardless, thank you very much.

@jcbrand
Copy link
Member

jcbrand commented Aug 11, 2022

Hi @based-a-tron, I'm very glad about the work you've done on this so far. Thanks!

Looking at your code, the main thing I would change would be to store the scram_keys in a Backboner|Skeletor Model.

Similarly to how the user_settings is created here:

user_settings = new Model({id});

The main advantage of using a Model is that the storage backend will be IndexedDB or localStorage depending on configuration and availability and it will match how everything else is stored.

I didn't see an existing model that looks like a place to store the keys, so I think a new one can be created for this.

One thing you didn't show, is how where you get the scram_keys and pass it as the password.
Where you do that, instead of simply reading it from localStorage, you would create the Model instance, and then call fetch on it, so that it gets populated with the data from the browser storage. If there were SCRAM keys in storage, your model will now be populated with them, and if not, you can let the user log in normally/manually, and then store the SCRAM keys on the model in your callback you created above.

I think it would be great if you made a PR with the changes you've made so far. If you could change it to use a Model and not write to localStorage directly, even better.

@based-a-tron
Copy link
Author

Yeah, I can do that. Give me a bit to sort everything out.

@based-a-tron
Copy link
Author

based-a-tron commented Aug 12, 2022

Looking at your code, the main thing I would change would be to store the scram_keys in a Backboner|Skeletor Model.

You should read this one again :)

@jcbrand
Copy link
Member

jcbrand commented Aug 12, 2022

🙈

@based-a-tron
Copy link
Author

Just a minor semantic detail here, how should we decide which username to select for the authentication, or should the client side logic be responsible for this?
I tend to prefer the latter.
But, in that case, whats the best way to signal to the business logic that the SCRAM login was successful?
In my hacky way of doing it, I was just reading the saved scram data from localstorage, like so:

    var scramDataFound = false;

    const savedScramData = localStorage.getItem('scram_keys');
    if (savedScramData) {
        scramDataFound = true;
        auto_password = JSON.parse(savedScramData);
        auto_username = localStorage.getItem('scram_login'); 
        start_converse();
    } else {
        // do semi-anonymous login stuff here
    }

So I dunno here. I dunno if we should expose the auto login data in a public facing API, so the business logic can select which username to login as, or what. In my specific case, I'd at least like a way to see if there is some SCRAM data saved before initializing converse, as if there is not, we try to run logic to login semi-anonymously (the user is supplied a captcha and some other information).
I'm personally ok with just hacking the client to get what I want out of it, but it's not a very elegant or "professional" solution.
I'd appreciate your input on this before I end up doing more work than necessary, thanks.

@jcbrand
Copy link
Member

jcbrand commented Aug 20, 2022

I think the best would be if you just make a PR with what you have so far and then we iterate from there. It's easier for me to give feedback in the PR based on the code, and I can also check out the branch and play with it locally.

jcbrand added a commit that referenced this issue Dec 27, 2022
- No need to create a new storage mechanism, just use `persistent`.
- Store SCRAM keys per JID
- Upon succesfull login, store the current session JID, so that we know who to fetch SCRAM keys for

Fixes #3001
@Neustradamus
Copy link

@jcbrand: Thanks!

Linked to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants