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

Minor refactoring on existing SCRAMSHA1AuthMechanism #92

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

rydnr
Copy link

@rydnr rydnr commented Dec 22, 2020

This is a minor refactoring on existing SCRAMSHA1AuthMechanism. Common logic is moved to a new parent class SCRAMSHAAuthMechanism. Descendant classes SCRAMSHA1AuthMechanism and SCRAMSHA256AuthMechanism only customize the hash function to use.

This PR doesn't implement the handshake mentioned in #87.

Common logic is moved to a new parent class SCRAMSHAAuthMechanism. Descendant classes SCRAMSHA1AuthMechanism and SCRAMSHA256AuthMechanism only customize the hash function to use.

This PR doesn't implement the handshake mentioned in pharo-nosql#87.
@rydnr rydnr changed the title This is a minor refactoring on existing SCRAMSHA1AuthMechanism. Minor refactoring on existing SCRAMSHA1AuthMechanism Dec 22, 2020
@tinchodias
Copy link
Contributor

Thanks for the contribution.
It looks good, I will merge it. But it'd be good to add a test using this new authentication method, even if some manual instructions are needed to setup the server.
Anyway, I think it's enough for the moment.

@tinchodias
Copy link
Contributor

The CI errors in Pharo 9 are unrelated to this change, and same tests run successfully in Pharo 8.

@tinchodias tinchodias merged commit 5fdbbd1 into pharo-nosql:master Jan 12, 2021
Comment on lines +2 to +3
authenticateAgainstAdmin
overrideDB := true
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, I realize this method has no senders and uses a variable that is not defined.
I will remove it.
@rydnr

Copy link
Contributor

Choose a reason for hiding this comment

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

done, in master

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.

None yet

2 participants