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 importation of db from FirstUse Auth #67

Merged
merged 10 commits into from
Mar 4, 2019

Conversation

leportella
Copy link
Collaborator

@leportella leportella commented Feb 21, 2019

As suggested on this TLJH issue, we should have a method to import data from FirstUse Authenticator in case a user would like to change authenticators.

However, Native Auth can return an empty user if the user is not sanitized (has commas or whitespaces) or have option for a password security check. Thus, I added an error to be raise, so the person should know that user won't be able to be created.

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

I think this should instead be a small script that can be run explicitly, instead of implicitly importing. We should only do the import once, and somehow explicitly - rather than at each startup. So on TLJH, you can run a script at install that detects if there is a passwords db, import it into nativeauthenticator, and then delete the old db if the import is successful. With this, we'll have to set the traitlet to true, then make sure hub starts successfully, and then set it to false - while somehow detecting if the import was successful.

In general, we should prefer explicit scripts for doing one-time data imports like this.

Does that make sense?

@leportella
Copy link
Collaborator Author

@yuvipanda not sure how to do this. Could you give me more details or an similar example, please?

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

As I tried to explain how to do this as a separate script (similar to https://jupyterhub.readthedocs.io/en/stable/reference/upgrading.html#upgrade-jupyterhub-databases) I realized it's going to be very complicated, and probably not worth it!

Instead, let's add another traitlet that allows us to delete the firstuse password file once we have successfully imported the users? This makes sure we don't leave around the old passwords lying around in, and also makes sure we are not importing it over and over again. How does that sound?

default=False,
help="Import users from FirstUse Authenticator database"
)
dbm_path = Unicode(
Copy link
Contributor

Choose a reason for hiding this comment

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

Call this firstuse_dbm_path to make it clear what is in the dbm file?

@leportella
Copy link
Collaborator Author

@yuvipanda I added the changes. The tests pass locally, but they don't pass on circleci. I can't imagine what could be wrong. Any ideia?

@yuvipanda
Copy link
Contributor

@leportella hmm, maybe you accidentally have a jupyterhub_config.py file that has correct path? Also do we need to manually add the '.db' suffix when deleting? It should be the same as the path passed in to us, right?

@leportella
Copy link
Collaborator Author

The thing is... for some reason dbm library doesn't accept namefiles with .db.
When I open/create a file called dbm.db it actually creates a file called dbm.db.db. And if try to open this file, it raises an error. So, the name file on the attribute should not have a .db in the end... which generates a lot of mess

screen shot 2019-02-26 at 22 00 25

@leportella
Copy link
Collaborator Author

@yuvipanda I tried to fix the tests using the full path of the file given by the Path but is not working still. Go back to this tomorrow

@yuvipanda
Copy link
Contributor

@leportella cool. CircleCI lets you ssh into the build system - https://circleci.com/docs/2.0/ssh-access-jobs/. Helps debug issues like this much easier!

One thing I'd recommend is to try construct the absolute path to the file first, rather than a relative path. Currently, the path to file is relative to 'current directory'. You can and should turn into an absolute path (starting with '/') with something like os.path.abspath(dbm_path). This should also help catch issues with different 'current directory' than what you expect.

@leportella
Copy link
Collaborator Author

@yuvipanda found the problem using the Circle CI ssh!
The problem is the different behavior of the library dbm depending on the operational systems. My mac is BDS based, so it adds the .db at the end, but the Circle CI system is not, so it doesn't. The problem is only on deleting the file, so I added a verification there :)

@yuvipanda yuvipanda merged commit 7c505ec into master Mar 4, 2019
@yuvipanda
Copy link
Contributor

Great Job, @leportella! \o/

@lambdaTotoro lambdaTotoro deleted the add-first-use-incorporation branch September 29, 2021 17:15
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.

2 participants