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

Adapt product registration to the new architecture #1146

Merged
merged 14 commits into from
Apr 19, 2024

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Apr 12, 2024

Another change in set that switch web UI from using dbus to HTTP API. As part of this change the rust code is added to provide that HTTP API and also adapted web UI. It also fixes all relevant js tests.

Things discussed but postponed (due to size of change and time constraints ) in this change was:

  1. move registration requirement from registration to product as it is highly coupled with it
  2. provide all changed data in websocket signal, so client does not need to do another http call to get correct data

Matching trello card: https://trello.com/c/2bTZOnpB

pub async fn registration_requirement(&self) -> Result<RegistrationRequirement, ServiceError> {
let requirement = self.registration_proxy.requirement().await?;
// unknown number can happen only if we do programmer mistake
let result: RegistrationRequirement = requirement.try_into().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

What about simple ? instead of unwrap()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

problem with ? is that it is not ServiceError, so I will need to convert that error somehow ( doable as I own error ), but I think it is not worth effort.

@jreidinger
Copy link
Contributor Author

I get this warning in FF using arch branch. @lslezak @imobachgs does it ring any bells to you?

Cookie “agamaToken” does not have a proper “SameSite” attribute value. Soon, cookies without the “SameSite” attribute or with an invalid value will be treated as “Lax”. This means that the cookie will no longer be sent in third-party contexts. If your application depends on this cookie being available in such contexts, please add the “SameSite=None“ attribute to it. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

@imobachgs
Copy link
Contributor

I get this warning in FF using arch branch. @lslezak @imobachgs does it ring any bells to you?

Cookie “agamaToken” does not have a proper “SameSite” attribute value. Soon, cookies without the “SameSite” attribute or with an invalid value will be treated as “Lax”. This means that the cookie will no longer be sent in third-party contexts. If your application depends on this cookie being available in such contexts, please add the “SameSite=None“ attribute to it. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

I would say we do not need to send the cookies in third-party contexts. We could easily set the SameSite value in the atuh_cookie_from_token function.

@jreidinger jreidinger marked this pull request as ready for review April 17, 2024 14:01
@imobachgs imobachgs changed the title Registration adapt Adapt product registration to the new architecture Apr 18, 2024
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

In general, it looks good. Just a few suggestions.

rust/agama-lib/src/product/client.rs Outdated Show resolved Hide resolved
async fn registration_requirement_changed_stream(
dbus: zbus::Connection,
) -> Result<impl Stream<Item = Event>, Error> {
// TODO: move registration requirement to product in dbus and so just one event will be needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

As we agreed, I will create a card for this.

/// Register product
///
/// * `state`: service state.
#[utoipa::path(post, path = "/software/registration", responses(
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 most probably use PUT, as you are replacing (or creating) the resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, if I use PUT, I will have to create it idempotent, which it is currently not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I am not sure. If you perform the same action with the same data several times, what changes? I would expect the result to be the same. But I am fine with using POST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, the change will be that after first successful registration the next one fails. Also we do not handle if you use different data. You have to first deregister and then can register with different data. We handle that automatic only when you change product.

rust/agama-server/src/software/web.rs Show resolved Hide resolved
rust/agama-server/src/software/web.rs Outdated Show resolved Hide resolved
web/src/client/software.js Show resolved Hide resolved
web/src/client/software.js Outdated Show resolved Hide resolved
web/src/client/software.js Outdated Show resolved Hide resolved
web/src/client/software.js Outdated Show resolved Hide resolved
web/src/client/software.js Outdated Show resolved Hide resolved
@jreidinger
Copy link
Contributor Author

JFYI testing of error response succeed:

registration_failure

@jreidinger jreidinger merged commit b43af77 into architecture_2024 Apr 19, 2024
4 checks passed
@jreidinger jreidinger deleted the registration_adapt branch April 19, 2024 06:35
imobachgs added a commit that referenced this pull request May 6, 2024
After a few months of work, it is time to merge the `architecture_2024`
branch into `master`. It is still a work-in-progress, but all the
efforts should be go now against that branch.

## Pull requests

* #1061
* #1064
* #1073
* #1074
* #1080
* #1089
* #1091
* #1092
* #1094
* #1095
* #1099
* #1100
* #1102
* #1103
* #1112
* #1114
* #1116
* #1117
* #1119
* #1120
* #1123
* #1126
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1136
* #1139
* #1140
* #1143
* #1146

## Other commits

* 8efa41f
* 9e2dec0
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
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.

3 participants