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

Update README.md to add some missing details in examples #3254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kidGodzilla
Copy link

I built an example and ran into a few small omissions in the examples here, that might make it difficult for a new developer not incredibly familiar with OAuth internals to figure out why their example doesn't work.

My recommended changes to README.md:

Each JoseKey.fromImportable call requires a key ID (parameter 2). Recommended change:

            JoseKey.fromImportable(process.env.PRIVATE_KEY_1, 'key1'),
            JoseKey.fromImportable(process.env.PRIVATE_KEY_2, 'key2'),
            JoseKey.fromImportable(process.env.PRIVATE_KEY_3, 'key3'),

Otherwise, it fails silently in testing (localhost) and fails with an error in production (because kid is missing on each key in jwks.json).


Scope requires atproto transition:generic for the code example that follows, would recommend adding the scope line back in as:

scope: 'atproto transition:generic',

Otherwise, the example that follows this (login) will not work, transition:generic is required for that.


This is required in the example given:

token_endpoint_auth_signing_alg: 'RS256',

Otherwise an error is thrown. Recommend adding this line.


Not included, if you wanted to include it, is a little in-memory store to complete the example.

// Simple In-memory session store (OAuth requirement)
class InMemoryStore {
    constructor() {
        this.storeData = {};
    }

    // stateStore methods
    async set(key, internalState) {
        this.storeData[key] = internalState;
    }

    async get(key) {
        return this.storeData[key] || undefined;
    }

    async del(key) {
        delete this.storeData[key];
    }
}
const stateStore = new InMemoryStore();
const sessionStore = new InMemoryStore();

But, I did not include this, you might have an existing library in mind or not want to give an easier option than the redis session store example.

Feel free to add if you think it improves things.

Improve code examples (some OAuth implementation details are missing in these examples)
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.

1 participant