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 an API #176

Merged
merged 33 commits into from Sep 13, 2022
Merged

Add an API #176

merged 33 commits into from Sep 13, 2022

Conversation

haaavk
Copy link
Contributor

@haaavk haaavk commented Oct 14, 2021

Usually I use Django REST Framework for API but in this case it could be overkill.
If there are plans for full API I can rewrite it using DRF.
I added additional argument to service.get_core_stats() to return less data.
Tokens can be managed only from Django admin.

@milesmcc
Copy link
Owner

Just wanted to thank you for putting in the work for this PR. I think that before we add an API we should stabilize what our method for handling new "custom" events will be, so while I'm not ready to merge quite yet, I think we'll be there soon. Going to leave this open for the time being.

@milesmcc milesmcc changed the title Api Add an API Oct 31, 2021
@milesmcc
Copy link
Owner

I propose we do this a bit differently: instead of having a complex API token setup, we simply add a "refreshable" API URL for every service (and in that URL is a token, but that token is tied to . In the service's management page, we can show that URL — along with a button to "regenerate" it. So for example, a service might have the following API URL: https://shynet.com/api/service/286ff19d-6062-4033-ba83-d27f3c0f8ef8/?token=fjkhdslfhlkjdsahf (where the token is stored inside the service model, and not another model).

This approach might be simpler because it allows us to not think about authorization as much, and it means that API tokens don't need to be coupled to users.

@haaavk
Copy link
Contributor Author

haaavk commented Nov 14, 2021

It will work for me because I'm the only user of my shynet instance and I have only a few services.
The problem begins with multi-users instance with a dozen services.
For example there are 10 services and 5 users.
All users get access to all services so everybody gets 10 links and add them to some kind of a client.
If we want to revoke the access to one user we need to refresh 10 tokens and resend them to 4 users and they need update them in a client.

@milesmcc
Copy link
Owner

I really want to keep Shynet small and focused, so maybe we just make it one refreshable API token per user instead? I really hesitate to introduce an additional model.

@haaavk
Copy link
Contributor Author

haaavk commented Nov 15, 2021

One token per user is a good idea. I will apply changes.
Should I add new view for API token or add it to security view or leave it to django admin?

@milesmcc
Copy link
Owner

milesmcc commented Nov 15, 2021

Great. Let’s create a new view for token management. We can also show the token on the service management page.

@haaavk
Copy link
Contributor Author

haaavk commented Dec 30, 2021

Is there anything else I should change in this PR?

@milesmcc
Copy link
Owner

Alright, thanks again for making this PR. I just played around with it in my local environment, and I think we're nearly there! Still, there are a few more things we need:

  • We need some documentation about how the API works. I mostly mean in places like GUIDE.md, but I also mean in the interface itself. Right now if you visit the API page, it has no context — we need to explain what is going on. For example, we should explain that this is the user's personal API token.
  • I don't think there should be a separate API tab. Perhaps we put it under "Account" or "Security"?
  • I'm not sure it makes sense to put the API key without any context on the service management page. Instead, I think we should provide the link to the API where the user can access that service's information. I think that would be more directly useful.

I can make the interface look a bit more consistent/prettier myself before we merge, so don't worry too much about that. Thanks!

@haaavk
Copy link
Contributor Author

haaavk commented Jan 5, 2022

I moved API token info to security tab and rename it to 'Personal API token'.
I added some basic API usage description to service management page and GUIDE.md.

@haaavk
Copy link
Contributor Author

haaavk commented Apr 14, 2022

I tried to add django-cors-headers but it generated conflict in poetry.lock.
I'm not sure how to resolve this conflict.

@milesmcc
Copy link
Owner

In my local dev environment, I get the following error when I try to query the API for a particular service:

ERROR Internal Server Error: /api/dashboard/
Traceback (most recent call last):
  File "/Users/miles/Library/Caches/pypoetry/virtualenvs/shynet--o1YXJxF-py3.9/lib/python3.9/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/Users/miles/Library/Caches/pypoetry/virtualenvs/shynet--o1YXJxF-py3.9/lib/python3.9/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/miles/Library/Caches/pypoetry/virtualenvs/shynet--o1YXJxF-py3.9/lib/python3.9/site-packages/django/views/generic/base.py", line 70, in view
    return self.dispatch(request, *args, **kwargs)
  File "/Users/miles/Source/shynet/shynet/api/mixins.py", line 23, in dispatch
    return super().dispatch(request, *args, **kwargs)
  File "/Users/miles/Library/Caches/pypoetry/virtualenvs/shynet--o1YXJxF-py3.9/lib/python3.9/site-packages/django/views/generic/base.py", line 98, in dispatch
    return handler(request, *args, **kwargs)
  File "/Users/miles/Source/shynet/shynet/api/views.py", line 38, in get
    services_data = [
  File "/Users/miles/Source/shynet/shynet/api/views.py", line 43, in <listcomp>
    'stats': s.get_core_stats(start, end, basic),
TypeError: get_core_stats() takes from 1 to 3 positional arguments but 4 were given

@milesmcc
Copy link
Owner

@milesmcc
Copy link
Owner

Once that error is fixed, I think we're good to go.

@haaavk
Copy link
Contributor Author

haaavk commented Aug 28, 2022

Sorry for this. Commit was lost in different branch somehow.

shynet/dashboard/views.py Outdated Show resolved Hide resolved
@milesmcc milesmcc merged commit 1280433 into milesmcc:master Sep 13, 2022
@haaavk haaavk deleted the api branch November 8, 2022 08:49
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