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.

Sign up for GitHub

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

Jump to bottom

feat(openHEXA): add openHEXA client #49

Merged
merged 4 commits into from
Aug 27, 2024
Merged

feat(openHEXA): add openHEXA client #49

merged 4 commits into from
Aug 27, 2024

Conversation

cheikhgwane
Copy link
Contributor

@cheikhgwane cheikhgwane commented Aug 8, 2024 •
edited
Loading

@cheikhgwane cheikhgwane force-pushed the feat/hexa-client branch 3 times, most recently from 27ffa0c to 4ef934c Compare August 8, 2024 14:52
@cheikhgwane cheikhgwane requested review from nazarfil and removed request for nazarfil August 8, 2024 15:22
nazarfil
nazarfil reviewed Aug 16, 2024
View reviewed changes
openhexa/toolbox/hexa/api.py Outdated
elif with_token:
self.session.headers.update({"Authorization": f"Bearer {with_token}"})
try:
self.query("""query{me {user {id}}}""")
Copy link
Contributor

@nazarfil nazarfil Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This won't work if MFA is enabled and you login , need to remove this line or add an if else block for MFA, or tell user to disable MFA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it's not the opposite ? IMO it's the auth with username/password that would require the 2FA and not the token auth.

Copy link
Contributor Author

@cheikhgwane cheikhgwane Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@qgerome Indeed the login when 2FA is enabled the login with username/password will not work and with token we cannot query the User ("""query{me {user {id}}}""") because of :

request.user.is_verified()

(no attribute is_verified). I propose to update the condition

if has_configured_two_factor(request.user) and getattr(request, "bypass_two_factor", False):..

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I have arrived to same code blocks as Cheick when i was looking into why it wasn't working for Jipe and Martin at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not skip 2fa on backend side, as it removes the point of having one. To use the toolbox in the pipeline with the token the user with 2FA enabled, I would suggest to avoid executing the "query me" to check if he is authenticated
It fits better with what we offer

@cheikhgwane cheikhgwane force-pushed the feat/hexa-client branch 2 times, most recently from 6456c1b to be6422e Compare August 20, 2024 10:43
mcrimi
mcrimi reviewed Aug 20, 2024
View reviewed changes
docs/hexa.md
pip install openhexa.toolbox
```

## [Example](#)
Copy link

Choose a reason for hiding this comment

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

Since the client is quite lightweight, I think it would be beneficial to include additional GraphQL query examples.

I'd propose adding the following:

1.	Getting the details of a given connection
2.	Browsing datasets and downloading a file

What do you think, @qgerome?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i will add them, thx.

qgerome
qgerome requested changes Aug 22, 2024
View reviewed changes
openhexa/toolbox/hexa/api.py Outdated
elif with_token:
self.session.headers.update({"Authorization": f"Bearer {with_token}"})
try:
self.query("""query{me {user {id}}}""")
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it's not the opposite ? IMO it's the auth with username/password that would require the 2FA and not the token auth.

openhexa/toolbox/hexa/hexa.py Outdated
when a login or authentication error

Examples:
>>> from openhexa.toolbox.hexa import openHEXA
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, it's OpenHEXA per your file

openhexa/toolbox/hexa/hexa.py Outdated
or

>>> from openhexa.toolbox.hexa import openHEXA
>>> hexa = openHEXA(server_url="http://iaso-staging.bluesquare.org",token="token")
Copy link
Member

Choose a reason for hiding this comment

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

What is this URL ?

docs/hexa.md Outdated
# We can authenticate using username / password
hexa_client = OpenHEXA("https://app.demo.openhexa.org", username="username", password="password")

# You can also use the token provided by OpenHEXA on the pipelines but make two-factor authentication is disabled else it will not work
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# You can also use the token provided by OpenHEXA on the pipelines but make two-factor authentication is disabled else it will not work
# You can also use the token provided by OpenHEXA on the pipelines but make sure two-factor authentication is disabled for it to work

docs/hexa.md Outdated

# You can also use the token provided by OpenHEXA on the pipelines but make two-factor authentication is disabled else it will not work
hexa_client = OpenHEXA("https://app.demo.openhexa.org", token="token")
page=1,per_page=10
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what it does there

@cheikhgwane cheikhgwane force-pushed the feat/hexa-client branch 2 times, most recently from 92a68ff to 32e9b4e Compare August 22, 2024 13:30
qgerome
qgerome requested changes Aug 26, 2024
View reviewed changes
openhexa/toolbox/hexa/api.py Outdated
)
resp.raise_for_status()
data = resp.json()["data"]
if data["login"]["success"]:
Copy link
Member

Choose a reason for hiding this comment

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

The correct way to ensure that the person has no 2FA is to check the errors on the result of the login mutation.

@qgerome qgerome merged commit 5d23150 into main Aug 27, 2024
2 checks passed
@qgerome qgerome mentioned this pull request Aug 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcrimi mcrimi mcrimi left review comments

@nazarfil nazarfil nazarfil approved these changes

@qgerome qgerome qgerome approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cheikhgwane @qgerome @mcrimi @nazarfil